[PATCH v3 0/2] clocksource: Add renesas-ostm timer driver

[PATCH v3 0/2] clocksource: Add renesas-ostm timer driver

[Chris Brandt]

This patch set adds a new clocksource driver that uses the OS Timer
(OSTM) that exists in the R7S72100 (RZ/A1) SoC.

The operation of the driver was tested with a simple user application
that does multiple calls to nanosleep() and gettimeofday().

The purpose of adding this driver is to get better time keeping
accuracy over the default MTU2 clocksource timer.

v3:
* Changed ostm@fcfec000 to timer@fcfec000
* Added power-domains to nodes

v2:
* The biggest change was now the channels are independent of each
  other and have separate nodes in the DT. The first probed will
  be set up as a clock source, and any additional channels probed
  will become a clock event.


Chris Brandt (2):
  dt-bindings: document renesas-ostm timer
  clocksource: Add renesas-ostm timer driver

 .../devicetree/bindings/timer/renesas,ostm.txt     |  30 ++
 arch/arm/mach-shmobile/Kconfig                     |   1 +
 drivers/clocksource/Kconfig                        |  12 +
 drivers/clocksource/Makefile                       |   1 +
 drivers/clocksource/renesas-ostm.c                 | 349 +++++++++++++++++++++
 5 files changed, 393 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/renesas,ostm.txt
 create mode 100644 drivers/clocksource/renesas-ostm.c

-- 
2.10.1

[PATCH v3 1/2] dt-bindings: document renesas-ostm timer

[Chris Brandt]

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
* changed ostm@fcfec000 to timer@fcfec000 in example
* added power-domains in example
v2:
* remove sw implementation specific portions
---
 .../devicetree/bindings/timer/renesas,ostm.txt     | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/renesas,ostm.txt

diff --git a/Documentation/devicetree/bindings/timer/renesas,ostm.txt b/Documentation/devicetree/bindings/timer/renesas,ostm.txt
new file mode 100644
index 0000000..be3ae0f
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/renesas,ostm.txt
@@ -0,0 +1,30 @@
+* Renesas OS Timer (OSTM)
+
+The OSTM is a multi-channel 32-bit timer/counter with fixed clock
+source that can operate in either interval count down timer or free-running
+compare match mode.
+
+Channels are independent from each other.
+
+Required Properties:
+
+  - compatible: must be one or more of the following:
+    - "renesas,r7s72100-ostm" for the r7s72100 OSTM
+    - "renesas,ostm" for any OSTM
+		This is a fallback for the above renesas,*-ostm entries
+
+  - reg: base address and length of the register block for a timer channel.
+
+  - interrupts: interrupt specifier for the timer channel.
+
+  - clocks: clock specifier for the timer channel.
+
+Example: R7S72100 (RZ/A1H) OSTM node
+
+	ostm0: timer@fcfec000 {
+		compatible = "renesas,r7s72100-ostm", "renesas,ostm";
+		reg = <0xfcfec000 0x30>;
+		interrupts = <GIC_SPI 102 IRQ_TYPE_EDGE_RISING>;
+		clocks = <&mstp5_clks R7S72100_CLK_OSTM0>;
+		power-domains = <&cpg_clocks>;
+	};
-- 
2.10.1

[PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Chris Brandt]

This patch adds a OSTM driver for the Renesas architecture.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

---
v2:
* changed implementation to be independent channel nodes
---
 arch/arm/mach-shmobile/Kconfig     |   1 +
 drivers/clocksource/Kconfig        |  12 ++
 drivers/clocksource/Makefile       |   1 +
 drivers/clocksource/renesas-ostm.c | 349 +++++++++++++++++++++++++++++++++++++
 4 files changed, 363 insertions(+)
 create mode 100644 drivers/clocksource/renesas-ostm.c

diff --git a/arch/arm/mach-shmobile/Kconfig b/arch/arm/mach-shmobile/Kconfig
index 2bb4b09..b928634 100644
--- a/arch/arm/mach-shmobile/Kconfig
+++ b/arch/arm/mach-shmobile/Kconfig
@@ -57,6 +57,7 @@ config ARCH_R7S72100
 	select PM
 	select PM_GENERIC_DOMAINS
 	select SYS_SUPPORTS_SH_MTU2
+	select SYS_SUPPORTS_RENESAS_OSTM
 
 config ARCH_R8A73A4
 	bool "R-Mobile APE6 (R8A73A40)"
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 4866f7a..95c8d56 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -431,6 +431,9 @@ config MTK_TIMER
 config SYS_SUPPORTS_SH_MTU2
         bool
 
+config SYS_SUPPORTS_RENESAS_OSTM
+        bool
+
 config SYS_SUPPORTS_SH_TMU
         bool
 
@@ -467,6 +470,15 @@ config SH_TIMER_MTU2
 	  Timer Pulse Unit 2 (MTU2) hardware available on SoCs from Renesas.
 	  This hardware comes with 16 bit-timer registers.
 
+config RENESAS_OSTM
+	bool "Renesas OSTM timer driver" if COMPILE_TEST
+	depends on GENERIC_CLOCKEVENTS
+	select CLKSRC_MMIO
+	default SYS_SUPPORTS_RENESAS_OSTM
+	help
+	  This enables the build of the OSTM timer driver.
+	  It creates a clock source and clock event device.
+
 config SH_TIMER_TMU
 	bool "Renesas TMU timer driver" if COMPILE_TEST
 	depends on GENERIC_CLOCKEVENTS
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index a14111e..bbd163b 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC)	+= cs5535-clockevt.o
 obj-$(CONFIG_CLKSRC_JCORE_PIT)		+= jcore-pit.o
 obj-$(CONFIG_SH_TIMER_CMT)	+= sh_cmt.o
 obj-$(CONFIG_SH_TIMER_MTU2)	+= sh_mtu2.o
+obj-$(CONFIG_RENESAS_OSTM)	+= renesas-ostm.o
 obj-$(CONFIG_SH_TIMER_TMU)	+= sh_tmu.o
 obj-$(CONFIG_EM_TIMER_STI)	+= em_sti.o
 obj-$(CONFIG_CLKBLD_I8253)	+= i8253.o
diff --git a/drivers/clocksource/renesas-ostm.c b/drivers/clocksource/renesas-ostm.c
new file mode 100644
index 0000000..37f2461
--- /dev/null
+++ b/drivers/clocksource/renesas-ostm.c
@@ -0,0 +1,349 @@
+/*
+ * Renesas Timer Support - OSTM
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/sched_clock.h>
+
+/*
+ * The OSTM contains independent channels.
+ * The first OSTM channel probed will be set up as a free running
+ * clocksource. Additionally we will use this clocksource for the system
+ * schedule timer sched_clock().
+ *
+ * The second (or more) channel probed will be set up as an interrupt
+ * driven clock event.
+ */
+
+struct ostm_device {
+	struct platform_device *pdev;
+
+	int irq;
+	struct clk *clk;
+	unsigned long rate;
+	void __iomem *base;
+	unsigned long ticks_per_jiffy;
+	struct clock_event_device ced;
+};
+
+static void __iomem *system_clock;	/* For sched_clock() */
+
+/* OSTM REGISTERS */
+#define	OSTM_CMP		0x000	/* RW,32 */
+#define	OSTM_CNT		0x004	/* R,32 */
+#define	OSTM_TE			0x010	/* R,8 */
+#define	OSTM_TS			0x014	/* W,8 */
+#define	OSTM_TT			0x018	/* W,8 */
+#define	OSTM_CTL		0x020	/* RW,8 */
+
+#define	TE			0x01
+#define	TS			0x01
+#define	TT			0x01
+#define	CTL_PERIODIC		0x00
+#define	CTL_ONESHOT		0x02
+#define	CTL_FREERUN		0x02
+
+static struct ostm_device *ced_to_ostm(struct clock_event_device *ced)
+{
+	return container_of(ced, struct ostm_device, ced);
+}
+
+static int __init ostm_init_clksrc(struct ostm_device *ostm)
+{
+	int ret;
+
+	/* irq not used (clock sources don't use interrupts) */
+
+	/* stop counter */
+	iowrite8(TT, ostm->base + OSTM_TT);
+	while (ioread8(ostm->base + OSTM_TE) & TE)
+		;
+
+	/* setup as freerun */
+	iowrite32(0, ostm->base + OSTM_CMP);
+	iowrite8(CTL_FREERUN, ostm->base + OSTM_CTL);
+	iowrite8(TS, ostm->base + OSTM_TS);
+
+	/* register */
+	ret = clocksource_mmio_init(ostm->base + OSTM_CNT,
+			"ostm", ostm->rate,
+			300, 32, clocksource_mmio_readl_up);
+
+	return ret;
+}
+
+static u64 notrace ostm_read_sched_clock(void)
+{
+	return ioread32(system_clock);
+}
+
+static int __init ostm_init_sched_clock(struct ostm_device *ostm)
+{
+	unsigned long flags;
+
+	system_clock = ostm->base + OSTM_CNT;
+	local_irq_save(flags);
+	local_irq_disable();
+	sched_clock_register(ostm_read_sched_clock, 32, ostm->rate);
+	local_irq_restore(flags);
+
+	return 0;
+}
+
+static void ostm_clkevt_timer_stop(struct ostm_device *ostm)
+{
+	if (ioread8(ostm->base + OSTM_TE) & TE) {
+		iowrite8(TT, ostm->base + OSTM_TT);
+		while (ioread8(ostm->base + OSTM_TE) & TE)
+			;
+	}
+}
+
+static int ostm_clock_event_next(unsigned long delta,
+				     struct clock_event_device *ced)
+{
+	struct ostm_device *ostm = ced_to_ostm(ced);
+
+	WARN_ON(!clockevent_state_oneshot(ced));
+
+	ostm_clkevt_timer_stop(ostm);
+
+	iowrite32(delta, ostm->base + OSTM_CMP);
+	iowrite8(CTL_ONESHOT, ostm->base + OSTM_CTL);
+	iowrite8(TS, ostm->base + OSTM_TS);
+
+	return 0;
+}
+
+static int ostm_shutdown(struct clock_event_device *ced)
+{
+	struct ostm_device *ostm = ced_to_ostm(ced);
+
+	ostm_clkevt_timer_stop(ostm);
+
+	return 0;
+}
+static int ostm_set_periodic(struct clock_event_device *ced)
+{
+	struct ostm_device *ostm = ced_to_ostm(ced);
+
+	if (clockevent_state_oneshot(ced) || clockevent_state_periodic(ced))
+		ostm_clkevt_timer_stop(ostm);
+
+	iowrite32(ostm->ticks_per_jiffy - 1, ostm->base + OSTM_CMP);
+	iowrite8(CTL_PERIODIC, ostm->base + OSTM_CTL);
+	iowrite8(TS, ostm->base + OSTM_TS);
+
+	return 0;
+}
+
+static int ostm_set_oneshot(struct clock_event_device *ced)
+{
+	struct ostm_device *ostm = ced_to_ostm(ced);
+
+	ostm_clkevt_timer_stop(ostm);
+
+	return 0;
+}
+
+static irqreturn_t ostm_timer_interrupt(int irq, void *dev_id)
+{
+	struct ostm_device *ostm = dev_id;
+
+	if (clockevent_state_oneshot(&ostm->ced))
+		ostm_clkevt_timer_stop(ostm);
+
+	/* notify clockevent layer */
+	if (ostm->ced.event_handler)
+		ostm->ced.event_handler(&ostm->ced);
+
+	return IRQ_HANDLED;
+}
+
+static int __init ostm_init_clkevt(struct ostm_device *ostm)
+{
+	struct clock_event_device *ced = &ostm->ced;
+	int ret = -ENXIO;
+
+	ret = request_irq(ostm->irq, ostm_timer_interrupt,
+			  IRQF_TIMER | IRQF_IRQPOLL | IRQF_NOBALANCING,
+			  dev_name(&ostm->pdev->dev), ostm);
+	if (ret) {
+		dev_err(&ostm->pdev->dev, "failed to request irq\n");
+		return ret;
+	}
+
+	ced->name = "ostm";
+	ced->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC;
+	ced->set_state_shutdown = ostm_shutdown;
+	ced->set_state_periodic = ostm_set_periodic;
+	ced->set_state_oneshot = ostm_set_oneshot;
+	ced->set_next_event = ostm_clock_event_next;
+	ced->shift = 32;
+	ced->rating = 300;
+	ced->cpumask = cpumask_of(0);
+	clockevents_config_and_register(ced, ostm->rate, 0xf, 0xffffffff);
+
+	return 0;
+}
+
+static int __init ostm_probe(struct platform_device *pdev)
+{
+	struct ostm_device *ostm;
+	struct resource *res;
+	int ret = -EFAULT;
+
+	if (!is_early_platform_device(pdev)) {
+		pm_runtime_set_active(&pdev->dev);
+		pm_runtime_enable(&pdev->dev);
+	}
+
+	ostm = platform_get_drvdata(pdev);
+	if (ostm) {
+		dev_info(&pdev->dev, "kept as earlytimer\n");
+		ret = 0;
+		goto out;
+	}
+
+	ostm = kzalloc(sizeof(*ostm), GFP_KERNEL);
+	if (!ostm) {
+		dev_err(&pdev->dev, "failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	ostm->pdev = pdev;
+	platform_set_drvdata(ostm->pdev, ostm);
+
+	res = platform_get_resource(ostm->pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&ostm->pdev->dev, "failed to get I/O memory\n");
+		goto err;
+	}
+
+	ostm->base = ioremap_nocache(res->start, resource_size(res));
+	if (!ostm->base) {
+		dev_err(&ostm->pdev->dev, "failed to remap I/O memory\n");
+		goto err;
+	}
+
+	ostm->irq = platform_get_irq(ostm->pdev, 0);
+	if (ostm->irq < 0) {
+		dev_err(&ostm->pdev->dev, "failed to get irq\n");
+		goto err;
+	}
+
+	ostm->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(ostm->clk)) {
+		dev_err(&ostm->pdev->dev, "failed to get clock\n");
+		ostm->clk = NULL;
+		goto err;
+	}
+
+	ret = clk_prepare_enable(ostm->clk);
+	if (ret) {
+		dev_err(&ostm->pdev->dev, "failed to enable clock\n");
+		goto err;
+	}
+
+	ostm->rate = clk_get_rate(ostm->clk);
+	ostm->ticks_per_jiffy = (ostm->rate + HZ / 2) / HZ;
+
+	/* First probed device will be used as system clocksource */
+	if (!system_clock) {
+		/* use as clocksource */
+		ret = ostm_init_clksrc(ostm);
+
+		/* use as system scheduling clock */
+		if (!ret)
+			ret = ostm_init_sched_clock(ostm);
+
+		if (ret) {
+			dev_err(&pdev->dev, "failed to use as sched_clock\n");
+			system_clock = (void *)-1; /* prevent future attempts */
+			ret = 0;	/* still works as clocksource */
+		}
+
+		if (!ret)
+			dev_info(&pdev->dev, "used for clocksource\n");
+	} else {
+		/* use as clock event */
+		ret = ostm_init_clkevt(ostm);
+
+		if (!ret)
+			dev_info(&pdev->dev, "used for clock events\n");
+	}
+
+err:
+	if (ret) {
+		if (ostm->clk)
+			clk_disable_unprepare(ostm->clk);
+		if (ostm->base)
+			iounmap(ostm->base);
+		kfree(ostm);
+		platform_set_drvdata(pdev, NULL);
+		pm_runtime_idle(&pdev->dev);
+		return ret;
+	}
+
+	if (is_early_platform_device(pdev))
+		return ret;
+
+out:
+	pm_runtime_irq_safe(&pdev->dev);
+
+	return ret;
+}
+
+static int ostm_remove(struct platform_device *pdev)
+{
+	return -EBUSY;	/* cannot unregister clockevent */
+}
+
+static const struct of_device_id ostm_of_table[] __maybe_unused = {
+	{ .compatible = "renesas,ostm" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ostm_of_table);
+
+static struct platform_driver ostm_timer = {
+	.probe		= ostm_probe,
+	.remove		= ostm_remove,
+	.driver	= {
+		.name	= "ostm",
+		.of_match_table = of_match_ptr(ostm_of_table),
+	},
+};
+
+static int __init ostm_init(void)
+{
+	return platform_driver_register(&ostm_timer);
+}
+
+static void __exit ostm_exit(void)
+{
+	platform_driver_unregister(&ostm_timer);
+}
+
+early_platform_init("earlytimer", &ostm_timer);
+subsys_initcall(ostm_init);
+module_exit(ostm_exit);
+
+MODULE_AUTHOR("Chris Brandt");
+MODULE_DESCRIPTION("Renesas OSTM Timer Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.10.1

Re: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Daniel Lezcano]

On Mon, Jan 23, 2017 at 08:54:23AM -0500, Chris Brandt wrote:
> This patch adds a OSTM driver for the Renesas architecture.

As it is a new driver, please give technical details for the log.
 
Replace ioread/write by readl/writel in the code.

> Signed-off-by: Chris Brandt <chris.brandt-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> 
> ---
> v2:
> * changed implementation to be independent channel nodes
> ---
>  arch/arm/mach-shmobile/Kconfig     |   1 +
>  drivers/clocksource/Kconfig        |  12 ++
>  drivers/clocksource/Makefile       |   1 +
>  drivers/clocksource/renesas-ostm.c | 349 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 363 insertions(+)
>  create mode 100644 drivers/clocksource/renesas-ostm.c
> 
> diff --git a/arch/arm/mach-shmobile/Kconfig b/arch/arm/mach-shmobile/Kconfig
> index 2bb4b09..b928634 100644
> --- a/arch/arm/mach-shmobile/Kconfig
> +++ b/arch/arm/mach-shmobile/Kconfig
> @@ -57,6 +57,7 @@ config ARCH_R7S72100
>  	select PM
>  	select PM_GENERIC_DOMAINS
>  	select SYS_SUPPORTS_SH_MTU2
> +	select SYS_SUPPORTS_RENESAS_OSTM

- select SYS_SUPPORTS_RENESAS_OSTM
+ select RENESAS_OSTM

>  
>  config ARCH_R8A73A4
>  	bool "R-Mobile APE6 (R8A73A40)"
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 4866f7a..95c8d56 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -431,6 +431,9 @@ config MTK_TIMER
>  config SYS_SUPPORTS_SH_MTU2
>          bool
>  
> +config SYS_SUPPORTS_RENESAS_OSTM
> +        bool
> +

-config SYS_SUPPORTS_RENESAS_OSTM
-        bool
-

>  config SYS_SUPPORTS_SH_TMU
>          bool
>  
> @@ -467,6 +470,15 @@ config SH_TIMER_MTU2
>  	  Timer Pulse Unit 2 (MTU2) hardware available on SoCs from Renesas.
>  	  This hardware comes with 16 bit-timer registers.
>  
> +config RENESAS_OSTM
> +	bool "Renesas OSTM timer driver" if COMPILE_TEST
> +	depends on GENERIC_CLOCKEVENTS
> +	select CLKSRC_MMIO
> +	default SYS_SUPPORTS_RENESAS_OSTM

- default SYS_SUPPORTS_RENESAS_OSTM

Except I missing something, I don' the point of having this intermediate
config option.

> +	help
> +	  This enables the build of the OSTM timer driver.
> +	  It creates a clock source and clock event device.
> +
>  config SH_TIMER_TMU
>  	bool "Renesas TMU timer driver" if COMPILE_TEST
>  	depends on GENERIC_CLOCKEVENTS
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index a14111e..bbd163b 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC)	+= cs5535-clockevt.o
>  obj-$(CONFIG_CLKSRC_JCORE_PIT)		+= jcore-pit.o
>  obj-$(CONFIG_SH_TIMER_CMT)	+= sh_cmt.o
>  obj-$(CONFIG_SH_TIMER_MTU2)	+= sh_mtu2.o
> +obj-$(CONFIG_RENESAS_OSTM)	+= renesas-ostm.o
>  obj-$(CONFIG_SH_TIMER_TMU)	+= sh_tmu.o
>  obj-$(CONFIG_EM_TIMER_STI)	+= em_sti.o
>  obj-$(CONFIG_CLKBLD_I8253)	+= i8253.o
> diff --git a/drivers/clocksource/renesas-ostm.c b/drivers/clocksource/renesas-ostm.c
> new file mode 100644
> index 0000000..37f2461
> --- /dev/null
> +++ b/drivers/clocksource/renesas-ostm.c
> @@ -0,0 +1,349 @@
> +/*
> + * Renesas Timer Support - OSTM
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/clockchips.h>
> +#include <linux/interrupt.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>

Please remove everything module related here and below. This driver is not
supposed to be removed and it is compiled in with the Kconfig option.

> +#include <linux/pm_runtime.h>
> +#include <linux/sched_clock.h>
> +
> +/*
> + * The OSTM contains independent channels.
> + * The first OSTM channel probed will be set up as a free running
> + * clocksource. Additionally we will use this clocksource for the system
> + * schedule timer sched_clock().
> + *
> + * The second (or more) channel probed will be set up as an interrupt
> + * driven clock event.
> + */
> +
> +struct ostm_device {
> +	struct platform_device *pdev;
> +	int irq;
> +	struct clk *clk;
> +	unsigned long rate;
> +	void __iomem *base;
> +	unsigned long ticks_per_jiffy;
> +	struct clock_event_device ced;
> +};

You can probably reduce the size of this structure by removing some fields
which are used only at init time.

> +
> +static void __iomem *system_clock;	/* For sched_clock() */
> +
> +/* OSTM REGISTERS */
> +#define	OSTM_CMP		0x000	/* RW,32 */
> +#define	OSTM_CNT		0x004	/* R,32 */
> +#define	OSTM_TE			0x010	/* R,8 */
> +#define	OSTM_TS			0x014	/* W,8 */
> +#define	OSTM_TT			0x018	/* W,8 */
> +#define	OSTM_CTL		0x020	/* RW,8 */
> +
> +#define	TE			0x01
> +#define	TS			0x01
> +#define	TT			0x01
> +#define	CTL_PERIODIC		0x00
> +#define	CTL_ONESHOT		0x02
> +#define	CTL_FREERUN		0x02
> +
> +static struct ostm_device *ced_to_ostm(struct clock_event_device *ced)
> +{
> +	return container_of(ced, struct ostm_device, ced);
> +}
> +
> +static int __init ostm_init_clksrc(struct ostm_device *ostm)
> +{
> +	int ret;
> +
> +	/* irq not used (clock sources don't use interrupts) */
> +
> +	/* stop counter */

One line comment is usually in the network code. Please use multiline.

/*
 * Bla bla
 */

> +	iowrite8(TT, ostm->base + OSTM_TT);
> +	while (ioread8(ostm->base + OSTM_TE) & TE)
> +		;

Comment here for this dangerous loop. Explain why we can't be stuck here.

> +
> +	/* setup as freerun */
> +	iowrite32(0, ostm->base + OSTM_CMP);
> +	iowrite8(CTL_FREERUN, ostm->base + OSTM_CTL);
> +	iowrite8(TS, ostm->base + OSTM_TS);
> +
> +	/* register */
> +	ret = clocksource_mmio_init(ostm->base + OSTM_CNT,
> +			"ostm", ostm->rate,
> +			300, 32, clocksource_mmio_readl_up);
> +
> +	return ret;

return clocksource_mmio_init(...);

> +}
> +
> +static u64 notrace ostm_read_sched_clock(void)
> +{
> +	return ioread32(system_clock);
> +}
> +
> +static int __init ostm_init_sched_clock(struct ostm_device *ostm)
> +{
> +	unsigned long flags;
> +
> +	system_clock = ostm->base + OSTM_CNT;
> +	local_irq_save(flags);
> +	local_irq_disable();

1. local_irq_disable() is not needed, local_irq_save() saves the irq flags and
disables the irq.

2. Why do you need to disable the irq here ?

> +	sched_clock_register(ostm_read_sched_clock, 32, ostm->rate);
> +	local_irq_restore(flags);
> +
> +	return 0;
> +}
> +
> +static void ostm_clkevt_timer_stop(struct ostm_device *ostm)
> +{
> +	if (ioread8(ostm->base + OSTM_TE) & TE) {
> +		iowrite8(TT, ostm->base + OSTM_TT);
> +		while (ioread8(ostm->base + OSTM_TE) & TE)
> +			;

Same comment as above.

> +	}
> +}
> +
> +static int ostm_clock_event_next(unsigned long delta,
> +				     struct clock_event_device *ced)
> +{
> +	struct ostm_device *ostm = ced_to_ostm(ced);
> +
> +	WARN_ON(!clockevent_state_oneshot(ced));

Pointless check. It is up to the time framework to handle that and that is the
case.

> +
> +	ostm_clkevt_timer_stop(ostm);
> +
> +	iowrite32(delta, ostm->base + OSTM_CMP);
> +	iowrite8(CTL_ONESHOT, ostm->base + OSTM_CTL);
> +	iowrite8(TS, ostm->base + OSTM_TS);
> +
> +	return 0;
> +}
> +
> +static int ostm_shutdown(struct clock_event_device *ced)
> +{
> +	struct ostm_device *ostm = ced_to_ostm(ced);
> +
> +	ostm_clkevt_timer_stop(ostm);
> +
> +	return 0;
> +}
> +static int ostm_set_periodic(struct clock_event_device *ced)
> +{
> +	struct ostm_device *ostm = ced_to_ostm(ced);
> +
> +	if (clockevent_state_oneshot(ced) || clockevent_state_periodic(ced))
> +		ostm_clkevt_timer_stop(ostm);
> +
> +	iowrite32(ostm->ticks_per_jiffy - 1, ostm->base + OSTM_CMP);
> +	iowrite8(CTL_PERIODIC, ostm->base + OSTM_CTL);
> +	iowrite8(TS, ostm->base + OSTM_TS);
> +
> +	return 0;
> +}
> +
> +static int ostm_set_oneshot(struct clock_event_device *ced)
> +{
> +	struct ostm_device *ostm = ced_to_ostm(ced);
> +
> +	ostm_clkevt_timer_stop(ostm);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t ostm_timer_interrupt(int irq, void *dev_id)
> +{
> +	struct ostm_device *ostm = dev_id;
> +
> +	if (clockevent_state_oneshot(&ostm->ced))
> +		ostm_clkevt_timer_stop(ostm);
> +
> +	/* notify clockevent layer */
> +	if (ostm->ced.event_handler)
> +		ostm->ced.event_handler(&ostm->ced);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int __init ostm_init_clkevt(struct ostm_device *ostm)
> +{
> +	struct clock_event_device *ced = &ostm->ced;
> +	int ret = -ENXIO;
> +
> +	ret = request_irq(ostm->irq, ostm_timer_interrupt,
> +			  IRQF_TIMER | IRQF_IRQPOLL | IRQF_NOBALANCING,
> +			  dev_name(&ostm->pdev->dev), ostm);

	devm_request_irq

Are you sure the IRQF_NOBALANCING flag should be used ? By default the
interrupt is pinned to cpu0 below. If this timer is used as a broadcast timer
for a system with deep idle, you may want to add the DYNIRQ flag feature to
improve the wakeups on the system.

> +	if (ret) {
> +		dev_err(&ostm->pdev->dev, "failed to request irq\n");
> +		return ret;
> +	}
> +
> +	ced->name = "ostm";
> +	ced->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC;
> +	ced->set_state_shutdown = ostm_shutdown;
> +	ced->set_state_periodic = ostm_set_periodic;
> +	ced->set_state_oneshot = ostm_set_oneshot;
> +	ced->set_next_event = ostm_clock_event_next;
> +	ced->shift = 32;
> +	ced->rating = 300;
> +	ced->cpumask = cpumask_of(0);
> +	clockevents_config_and_register(ced, ostm->rate, 0xf, 0xffffffff);
> +
> +	return 0;
> +}
> +
> +static int __init ostm_probe(struct platform_device *pdev)
> +{
> +	struct ostm_device *ostm;
> +	struct resource *res;
> +	int ret = -EFAULT;
> +
> +	if (!is_early_platform_device(pdev)) {
> +		pm_runtime_set_active(&pdev->dev);
> +		pm_runtime_enable(&pdev->dev);
> +	}

Can you clarify why the 'early' is needed here ?

I don't see the pm_runtime_get/pm_runtime_put in the corresponding function.

What is the benefit of using pm_runtime in this driver ? Isn't the timer
supposed to be in an always-on power domain ?

> +	ostm = platform_get_drvdata(pdev);
> +	if (ostm) {
> +		dev_info(&pdev->dev, "kept as earlytimer\n");
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	ostm = kzalloc(sizeof(*ostm), GFP_KERNEL);

	devm_kzalloc ?

> +	if (!ostm) {
> +		dev_err(&pdev->dev, "failed to allocate memory\n");

A memory failure allocation always triggers a dumpstack (IIRC), so this
error message is pointless.

> +		return -ENOMEM;
> +	}
> +
> +	ostm->pdev = pdev;
> +	platform_set_drvdata(ostm->pdev, ostm);
> +
> +	res = platform_get_resource(ostm->pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&ostm->pdev->dev, "failed to get I/O memory\n");
> +		goto err;
> +	}
> +
> +	ostm->base = ioremap_nocache(res->start, resource_size(res));

	devm_ioremap_nocache ?

> +	if (!ostm->base) {
> +		dev_err(&ostm->pdev->dev, "failed to remap I/O memory\n");
> +		goto err;
> +	}
> +
> +	ostm->irq = platform_get_irq(ostm->pdev, 0);
> +	if (ostm->irq < 0) {
> +		dev_err(&ostm->pdev->dev, "failed to get irq\n");
> +		goto err;
> +	}
> +
> +	ostm->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(ostm->clk)) {
> +		dev_err(&ostm->pdev->dev, "failed to get clock\n");
> +		ostm->clk = NULL;
> +		goto err;
> +	}
> +
> +	ret = clk_prepare_enable(ostm->clk);
> +	if (ret) {
> +		dev_err(&ostm->pdev->dev, "failed to enable clock\n");
> +		goto err;
> +	}
> +
> +	ostm->rate = clk_get_rate(ostm->clk);
> +	ostm->ticks_per_jiffy = (ostm->rate + HZ / 2) / HZ;
> +
> +	/* First probed device will be used as system clocksource */
> +	if (!system_clock) {
> +		/* use as clocksource */
> +		ret = ostm_init_clksrc(ostm);
> +
> +		/* use as system scheduling clock */
> +		if (!ret)
> +			ret = ostm_init_sched_clock(ostm);
> +
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to use as sched_clock\n");
> +			system_clock = (void *)-1; /* prevent future attempts */
> +			ret = 0;	/* still works as clocksource */
> +		}

This error code check is unnecessary complex. ostm_init_sched_clock always
return zero.

	if (!system_clock) {
		system_clock = ostm_init_sched_clock(ostm);
		ret = ostm_init_clksrc(ostm)
		if (ret)
			dev_err("Failed to initialize the clocksource\n");
	} else {

		...
	}

This is not perfect but until I send the clksrc_of/clkevt_of split changes, we
have to deal with these kind of hacks.

> +		if (!ret)
> +			dev_info(&pdev->dev, "used for clocksource\n");
> +	} else {
> +		/* use as clock event */
> +		ret = ostm_init_clkevt(ostm);
> +
> +		if (!ret)
> +			dev_info(&pdev->dev, "used for clock events\n");
> +	}
> +
> +err:
> +	if (ret) {
> +		if (ostm->clk)
> +			clk_disable_unprepare(ostm->clk);
> +		if (ostm->base)
> +			iounmap(ostm->base);
> +		kfree(ostm);
> +		platform_set_drvdata(pdev, NULL);

As iomap, kzalloc were done with the devm, then the rollback will be handled
with the device framework.

> +		pm_runtime_idle(&pdev->dev);
> +		return ret;
> +	}
> +
> +	if (is_early_platform_device(pdev))
> +		return ret;
> +
> +out:
> +	pm_runtime_irq_safe(&pdev->dev);
> +
> +	return ret;
> +}
> +
> +static int ostm_remove(struct platform_device *pdev)
> +{
> +	return -EBUSY;	/* cannot unregister clockevent */
> +}
> +
> +static const struct of_device_id ostm_of_table[] __maybe_unused = {
> +	{ .compatible = "renesas,ostm" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ostm_of_table);
> +
> +static struct platform_driver ostm_timer = {
> +	.probe		= ostm_probe,
> +	.remove		= ostm_remove,
> +	.driver	= {
> +		.name	= "ostm",
> +		.of_match_table = of_match_ptr(ostm_of_table),
> +	},
> +};
> +
> +static int __init ostm_init(void)
> +{
> +	return platform_driver_register(&ostm_timer);
> +}
> +
> +static void __exit ostm_exit(void)
> +{
> +	platform_driver_unregister(&ostm_timer);
> +}
> +
> +early_platform_init("earlytimer", &ostm_timer);
> +subsys_initcall(ostm_init);
> +module_exit(ostm_exit);
> +
> +MODULE_AUTHOR("Chris Brandt");
> +MODULE_DESCRIPTION("Renesas OSTM Timer Driver");
> +MODULE_LICENSE("GPL v2");

Maybe you can try with builtin_platform ?

  -- Daniel

-- 

 <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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Daniel Lezcano]

On Mon, Jan 23, 2017 at 08:54:23AM -0500, Chris Brandt wrote:
> This patch adds a OSTM driver for the Renesas architecture.

As it is a new driver, please give technical details for the log.
 
Replace ioread/write by readl/writel in the code.

> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> 
> ---
> v2:
> * changed implementation to be independent channel nodes
> ---
>  arch/arm/mach-shmobile/Kconfig     |   1 +
>  drivers/clocksource/Kconfig        |  12 ++
>  drivers/clocksource/Makefile       |   1 +
>  drivers/clocksource/renesas-ostm.c | 349 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 363 insertions(+)
>  create mode 100644 drivers/clocksource/renesas-ostm.c
> 
> diff --git a/arch/arm/mach-shmobile/Kconfig b/arch/arm/mach-shmobile/Kconfig
> index 2bb4b09..b928634 100644
> --- a/arch/arm/mach-shmobile/Kconfig
> +++ b/arch/arm/mach-shmobile/Kconfig
> @@ -57,6 +57,7 @@ config ARCH_R7S72100
>  	select PM
>  	select PM_GENERIC_DOMAINS
>  	select SYS_SUPPORTS_SH_MTU2
> +	select SYS_SUPPORTS_RENESAS_OSTM

- select SYS_SUPPORTS_RENESAS_OSTM
+ select RENESAS_OSTM

>  
>  config ARCH_R8A73A4
>  	bool "R-Mobile APE6 (R8A73A40)"
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 4866f7a..95c8d56 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -431,6 +431,9 @@ config MTK_TIMER
>  config SYS_SUPPORTS_SH_MTU2
>          bool
>  
> +config SYS_SUPPORTS_RENESAS_OSTM
> +        bool
> +

-config SYS_SUPPORTS_RENESAS_OSTM
-        bool
-

>  config SYS_SUPPORTS_SH_TMU
>          bool
>  
> @@ -467,6 +470,15 @@ config SH_TIMER_MTU2
>  	  Timer Pulse Unit 2 (MTU2) hardware available on SoCs from Renesas.
>  	  This hardware comes with 16 bit-timer registers.
>  
> +config RENESAS_OSTM
> +	bool "Renesas OSTM timer driver" if COMPILE_TEST
> +	depends on GENERIC_CLOCKEVENTS
> +	select CLKSRC_MMIO
> +	default SYS_SUPPORTS_RENESAS_OSTM

- default SYS_SUPPORTS_RENESAS_OSTM

Except I missing something, I don' the point of having this intermediate
config option.

> +	help
> +	  This enables the build of the OSTM timer driver.
> +	  It creates a clock source and clock event device.
> +
>  config SH_TIMER_TMU
>  	bool "Renesas TMU timer driver" if COMPILE_TEST
>  	depends on GENERIC_CLOCKEVENTS
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index a14111e..bbd163b 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC)	+= cs5535-clockevt.o
>  obj-$(CONFIG_CLKSRC_JCORE_PIT)		+= jcore-pit.o
>  obj-$(CONFIG_SH_TIMER_CMT)	+= sh_cmt.o
>  obj-$(CONFIG_SH_TIMER_MTU2)	+= sh_mtu2.o
> +obj-$(CONFIG_RENESAS_OSTM)	+= renesas-ostm.o
>  obj-$(CONFIG_SH_TIMER_TMU)	+= sh_tmu.o
>  obj-$(CONFIG_EM_TIMER_STI)	+= em_sti.o
>  obj-$(CONFIG_CLKBLD_I8253)	+= i8253.o
> diff --git a/drivers/clocksource/renesas-ostm.c b/drivers/clocksource/renesas-ostm.c
> new file mode 100644
> index 0000000..37f2461
> --- /dev/null
> +++ b/drivers/clocksource/renesas-ostm.c
> @@ -0,0 +1,349 @@
> +/*
> + * Renesas Timer Support - OSTM
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/clockchips.h>
> +#include <linux/interrupt.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>

Please remove everything module related here and below. This driver is not
supposed to be removed and it is compiled in with the Kconfig option.

> +#include <linux/pm_runtime.h>
> +#include <linux/sched_clock.h>
> +
> +/*
> + * The OSTM contains independent channels.
> + * The first OSTM channel probed will be set up as a free running
> + * clocksource. Additionally we will use this clocksource for the system
> + * schedule timer sched_clock().
> + *
> + * The second (or more) channel probed will be set up as an interrupt
> + * driven clock event.
> + */
> +
> +struct ostm_device {
> +	struct platform_device *pdev;
> +	int irq;
> +	struct clk *clk;
> +	unsigned long rate;
> +	void __iomem *base;
> +	unsigned long ticks_per_jiffy;
> +	struct clock_event_device ced;
> +};

You can probably reduce the size of this structure by removing some fields
which are used only at init time.

> +
> +static void __iomem *system_clock;	/* For sched_clock() */
> +
> +/* OSTM REGISTERS */
> +#define	OSTM_CMP		0x000	/* RW,32 */
> +#define	OSTM_CNT		0x004	/* R,32 */
> +#define	OSTM_TE			0x010	/* R,8 */
> +#define	OSTM_TS			0x014	/* W,8 */
> +#define	OSTM_TT			0x018	/* W,8 */
> +#define	OSTM_CTL		0x020	/* RW,8 */
> +
> +#define	TE			0x01
> +#define	TS			0x01
> +#define	TT			0x01
> +#define	CTL_PERIODIC		0x00
> +#define	CTL_ONESHOT		0x02
> +#define	CTL_FREERUN		0x02
> +
> +static struct ostm_device *ced_to_ostm(struct clock_event_device *ced)
> +{
> +	return container_of(ced, struct ostm_device, ced);
> +}
> +
> +static int __init ostm_init_clksrc(struct ostm_device *ostm)
> +{
> +	int ret;
> +
> +	/* irq not used (clock sources don't use interrupts) */
> +
> +	/* stop counter */

One line comment is usually in the network code. Please use multiline.

/*
 * Bla bla
 */

> +	iowrite8(TT, ostm->base + OSTM_TT);
> +	while (ioread8(ostm->base + OSTM_TE) & TE)
> +		;

Comment here for this dangerous loop. Explain why we can't be stuck here.

> +
> +	/* setup as freerun */
> +	iowrite32(0, ostm->base + OSTM_CMP);
> +	iowrite8(CTL_FREERUN, ostm->base + OSTM_CTL);
> +	iowrite8(TS, ostm->base + OSTM_TS);
> +
> +	/* register */
> +	ret = clocksource_mmio_init(ostm->base + OSTM_CNT,
> +			"ostm", ostm->rate,
> +			300, 32, clocksource_mmio_readl_up);
> +
> +	return ret;

return clocksource_mmio_init(...);

> +}
> +
> +static u64 notrace ostm_read_sched_clock(void)
> +{
> +	return ioread32(system_clock);
> +}
> +
> +static int __init ostm_init_sched_clock(struct ostm_device *ostm)
> +{
> +	unsigned long flags;
> +
> +	system_clock = ostm->base + OSTM_CNT;
> +	local_irq_save(flags);
> +	local_irq_disable();

1. local_irq_disable() is not needed, local_irq_save() saves the irq flags and
disables the irq.

2. Why do you need to disable the irq here ?

> +	sched_clock_register(ostm_read_sched_clock, 32, ostm->rate);
> +	local_irq_restore(flags);
> +
> +	return 0;
> +}
> +
> +static void ostm_clkevt_timer_stop(struct ostm_device *ostm)
> +{
> +	if (ioread8(ostm->base + OSTM_TE) & TE) {
> +		iowrite8(TT, ostm->base + OSTM_TT);
> +		while (ioread8(ostm->base + OSTM_TE) & TE)
> +			;

Same comment as above.

> +	}
> +}
> +
> +static int ostm_clock_event_next(unsigned long delta,
> +				     struct clock_event_device *ced)
> +{
> +	struct ostm_device *ostm = ced_to_ostm(ced);
> +
> +	WARN_ON(!clockevent_state_oneshot(ced));

Pointless check. It is up to the time framework to handle that and that is the
case.

> +
> +	ostm_clkevt_timer_stop(ostm);
> +
> +	iowrite32(delta, ostm->base + OSTM_CMP);
> +	iowrite8(CTL_ONESHOT, ostm->base + OSTM_CTL);
> +	iowrite8(TS, ostm->base + OSTM_TS);
> +
> +	return 0;
> +}
> +
> +static int ostm_shutdown(struct clock_event_device *ced)
> +{
> +	struct ostm_device *ostm = ced_to_ostm(ced);
> +
> +	ostm_clkevt_timer_stop(ostm);
> +
> +	return 0;
> +}
> +static int ostm_set_periodic(struct clock_event_device *ced)
> +{
> +	struct ostm_device *ostm = ced_to_ostm(ced);
> +
> +	if (clockevent_state_oneshot(ced) || clockevent_state_periodic(ced))
> +		ostm_clkevt_timer_stop(ostm);
> +
> +	iowrite32(ostm->ticks_per_jiffy - 1, ostm->base + OSTM_CMP);
> +	iowrite8(CTL_PERIODIC, ostm->base + OSTM_CTL);
> +	iowrite8(TS, ostm->base + OSTM_TS);
> +
> +	return 0;
> +}
> +
> +static int ostm_set_oneshot(struct clock_event_device *ced)
> +{
> +	struct ostm_device *ostm = ced_to_ostm(ced);
> +
> +	ostm_clkevt_timer_stop(ostm);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t ostm_timer_interrupt(int irq, void *dev_id)
> +{
> +	struct ostm_device *ostm = dev_id;
> +
> +	if (clockevent_state_oneshot(&ostm->ced))
> +		ostm_clkevt_timer_stop(ostm);
> +
> +	/* notify clockevent layer */
> +	if (ostm->ced.event_handler)
> +		ostm->ced.event_handler(&ostm->ced);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int __init ostm_init_clkevt(struct ostm_device *ostm)
> +{
> +	struct clock_event_device *ced = &ostm->ced;
> +	int ret = -ENXIO;
> +
> +	ret = request_irq(ostm->irq, ostm_timer_interrupt,
> +			  IRQF_TIMER | IRQF_IRQPOLL | IRQF_NOBALANCING,
> +			  dev_name(&ostm->pdev->dev), ostm);

	devm_request_irq

Are you sure the IRQF_NOBALANCING flag should be used ? By default the
interrupt is pinned to cpu0 below. If this timer is used as a broadcast timer
for a system with deep idle, you may want to add the DYNIRQ flag feature to
improve the wakeups on the system.

> +	if (ret) {
> +		dev_err(&ostm->pdev->dev, "failed to request irq\n");
> +		return ret;
> +	}
> +
> +	ced->name = "ostm";
> +	ced->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC;
> +	ced->set_state_shutdown = ostm_shutdown;
> +	ced->set_state_periodic = ostm_set_periodic;
> +	ced->set_state_oneshot = ostm_set_oneshot;
> +	ced->set_next_event = ostm_clock_event_next;
> +	ced->shift = 32;
> +	ced->rating = 300;
> +	ced->cpumask = cpumask_of(0);
> +	clockevents_config_and_register(ced, ostm->rate, 0xf, 0xffffffff);
> +
> +	return 0;
> +}
> +
> +static int __init ostm_probe(struct platform_device *pdev)
> +{
> +	struct ostm_device *ostm;
> +	struct resource *res;
> +	int ret = -EFAULT;
> +
> +	if (!is_early_platform_device(pdev)) {
> +		pm_runtime_set_active(&pdev->dev);
> +		pm_runtime_enable(&pdev->dev);
> +	}

Can you clarify why the 'early' is needed here ?

I don't see the pm_runtime_get/pm_runtime_put in the corresponding function.

What is the benefit of using pm_runtime in this driver ? Isn't the timer
supposed to be in an always-on power domain ?

> +	ostm = platform_get_drvdata(pdev);
> +	if (ostm) {
> +		dev_info(&pdev->dev, "kept as earlytimer\n");
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	ostm = kzalloc(sizeof(*ostm), GFP_KERNEL);

	devm_kzalloc ?

> +	if (!ostm) {
> +		dev_err(&pdev->dev, "failed to allocate memory\n");

A memory failure allocation always triggers a dumpstack (IIRC), so this
error message is pointless.

> +		return -ENOMEM;
> +	}
> +
> +	ostm->pdev = pdev;
> +	platform_set_drvdata(ostm->pdev, ostm);
> +
> +	res = platform_get_resource(ostm->pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&ostm->pdev->dev, "failed to get I/O memory\n");
> +		goto err;
> +	}
> +
> +	ostm->base = ioremap_nocache(res->start, resource_size(res));

	devm_ioremap_nocache ?

> +	if (!ostm->base) {
> +		dev_err(&ostm->pdev->dev, "failed to remap I/O memory\n");
> +		goto err;
> +	}
> +
> +	ostm->irq = platform_get_irq(ostm->pdev, 0);
> +	if (ostm->irq < 0) {
> +		dev_err(&ostm->pdev->dev, "failed to get irq\n");
> +		goto err;
> +	}
> +
> +	ostm->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(ostm->clk)) {
> +		dev_err(&ostm->pdev->dev, "failed to get clock\n");
> +		ostm->clk = NULL;
> +		goto err;
> +	}
> +
> +	ret = clk_prepare_enable(ostm->clk);
> +	if (ret) {
> +		dev_err(&ostm->pdev->dev, "failed to enable clock\n");
> +		goto err;
> +	}
> +
> +	ostm->rate = clk_get_rate(ostm->clk);
> +	ostm->ticks_per_jiffy = (ostm->rate + HZ / 2) / HZ;
> +
> +	/* First probed device will be used as system clocksource */
> +	if (!system_clock) {
> +		/* use as clocksource */
> +		ret = ostm_init_clksrc(ostm);
> +
> +		/* use as system scheduling clock */
> +		if (!ret)
> +			ret = ostm_init_sched_clock(ostm);
> +
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to use as sched_clock\n");
> +			system_clock = (void *)-1; /* prevent future attempts */
> +			ret = 0;	/* still works as clocksource */
> +		}

This error code check is unnecessary complex. ostm_init_sched_clock always
return zero.

	if (!system_clock) {
		system_clock = ostm_init_sched_clock(ostm);
		ret = ostm_init_clksrc(ostm)
		if (ret)
			dev_err("Failed to initialize the clocksource\n");
	} else {

		...
	}

This is not perfect but until I send the clksrc_of/clkevt_of split changes, we
have to deal with these kind of hacks.

> +		if (!ret)
> +			dev_info(&pdev->dev, "used for clocksource\n");
> +	} else {
> +		/* use as clock event */
> +		ret = ostm_init_clkevt(ostm);
> +
> +		if (!ret)
> +			dev_info(&pdev->dev, "used for clock events\n");
> +	}
> +
> +err:
> +	if (ret) {
> +		if (ostm->clk)
> +			clk_disable_unprepare(ostm->clk);
> +		if (ostm->base)
> +			iounmap(ostm->base);
> +		kfree(ostm);
> +		platform_set_drvdata(pdev, NULL);

As iomap, kzalloc were done with the devm, then the rollback will be handled
with the device framework.

> +		pm_runtime_idle(&pdev->dev);
> +		return ret;
> +	}
> +
> +	if (is_early_platform_device(pdev))
> +		return ret;
> +
> +out:
> +	pm_runtime_irq_safe(&pdev->dev);
> +
> +	return ret;
> +}
> +
> +static int ostm_remove(struct platform_device *pdev)
> +{
> +	return -EBUSY;	/* cannot unregister clockevent */
> +}
> +
> +static const struct of_device_id ostm_of_table[] __maybe_unused = {
> +	{ .compatible = "renesas,ostm" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ostm_of_table);
> +
> +static struct platform_driver ostm_timer = {
> +	.probe		= ostm_probe,
> +	.remove		= ostm_remove,
> +	.driver	= {
> +		.name	= "ostm",
> +		.of_match_table = of_match_ptr(ostm_of_table),
> +	},
> +};
> +
> +static int __init ostm_init(void)
> +{
> +	return platform_driver_register(&ostm_timer);
> +}
> +
> +static void __exit ostm_exit(void)
> +{
> +	platform_driver_unregister(&ostm_timer);
> +}
> +
> +early_platform_init("earlytimer", &ostm_timer);
> +subsys_initcall(ostm_init);
> +module_exit(ostm_exit);
> +
> +MODULE_AUTHOR("Chris Brandt");
> +MODULE_DESCRIPTION("Renesas OSTM Timer Driver");
> +MODULE_LICENSE("GPL v2");

Maybe you can try with builtin_platform ?

  -- Daniel

-- 

 <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

RE: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Chris Brandt]

Hello Daniel,

Thank you for the review.
Your suggestions were very helpful to me.

I basically made all the changes, but I had some questions on
the Kconfig and multi-line commenting.


On Monday, January 23, 2017, Daniel Lezcano wrote:
> > This patch adds a OSTM driver for the Renesas architecture.
> 
> As it is a new driver, please give technical details for the log.

OK.


> Replace ioread/write by readl/writel in the code.

OK.


> > @@ -467,6 +470,15 @@ config SH_TIMER_MTU2
> >  	  Timer Pulse Unit 2 (MTU2) hardware available on SoCs from Renesas.
> >  	  This hardware comes with 16 bit-timer registers.
> >
> > +config RENESAS_OSTM
> > +	bool "Renesas OSTM timer driver" if COMPILE_TEST
> > +	depends on GENERIC_CLOCKEVENTS
> > +	select CLKSRC_MMIO
> > +	default SYS_SUPPORTS_RENESAS_OSTM
> 
> - default SYS_SUPPORTS_RENESAS_OSTM
> 
> Except I missing something, I don' the point of having this intermediate
> config option.

I was basically following what all the other clocksource drivers do.
Selecting an ARCH will then set SYS_SUPPORTS_xxTIMERxx which will then
force xxTIMERxx=y.
But, if "COMPILE_TEST" is set, you can choose what clocksource timers
you want to build in.

Is your suggestion to do away with the COMPILE_TEST ability and
just force RENESAS_OSTM=y if ARCH_R7S72100 is selected?



> > +#include <linux/module.h>
> 
> Please remove everything module related here and below. This driver is not
> supposed to be removed and it is compiled in with the Kconfig option.

Good point. I will remove.

I guess if you can't compile it as a module, you don't need things like
'MODULE_LICENSE'.


> > +struct ostm_device {
> > +	struct platform_device *pdev;
> > +	int irq;
> > +	struct clk *clk;
> > +	unsigned long rate;
> > +	void __iomem *base;
> > +	unsigned long ticks_per_jiffy;
> > +	struct clock_event_device ced;
> > +};
> 
> You can probably reduce the size of this structure by removing some fields
> which are used only at init time.

Looks like I can get rid of 'clk', 'irq' and 'rate'.
Thanks.



> > +static int __init ostm_init_clksrc(struct ostm_device *ostm) {
> > +	int ret;
> > +
> > +	/* irq not used (clock sources don't use interrupts) */
> > +
> > +	/* stop counter */
> 
> One line comment is usually in the network code. Please use multiline.
> 
> /*
>  * Bla bla
>  */

I'm confused. Do you mean:

A) use multiline formatting for every single-line comment throughout
   the file?

B) use multiline for any place where you have 2 or more single-line
   comments back to back?


> > +	iowrite8(TT, ostm->base + OSTM_TT);
> > +	while (ioread8(ostm->base + OSTM_TE) & TE)
> > +		;
> 
> Comment here for this dangerous loop. Explain why we can't be stuck here.

OK, I'll add something in as it's pretty safe.

> 
> > +
> > +	/* setup as freerun */
> > +	iowrite32(0, ostm->base + OSTM_CMP);
> > +	iowrite8(CTL_FREERUN, ostm->base + OSTM_CTL);
> > +	iowrite8(TS, ostm->base + OSTM_TS);
> > +
> > +	/* register */
> > +	ret = clocksource_mmio_init(ostm->base + OSTM_CNT,
> > +			"ostm", ostm->rate,
> > +			300, 32, clocksource_mmio_readl_up);
> > +
> > +	return ret;
> 
> return clocksource_mmio_init(...);

OK.


> > +static int __init ostm_init_sched_clock(struct ostm_device *ostm) {
> > +	unsigned long flags;
> > +
> > +	system_clock = ostm->base + OSTM_CNT;
> > +	local_irq_save(flags);
> > +	local_irq_disable();
> 
> 1. local_irq_disable() is not needed, local_irq_save() saves the irq flags
> and disables the irq.
> 
> 2. Why do you need to disable the irq here ?

OK, I see that none of the other drivers are disabling irq, so I'll
take that code out.
Thanks.


> > +static int ostm_clock_event_next(unsigned long delta,
> > +				     struct clock_event_device *ced) {
> > +	struct ostm_device *ostm = ced_to_ostm(ced);
> > +
> > +	WARN_ON(!clockevent_state_oneshot(ced));
> 
> Pointless check. It is up to the time framework to handle that and that is
> the case.


OK. I saw other drivers doing it so I thought it was my responsibility.
I'll take that lineout.



> > +static int __init ostm_init_clkevt(struct ostm_device *ostm) {
> > +	struct clock_event_device *ced = &ostm->ced;
> > +	int ret = -ENXIO;
> > +
> > +	ret = request_irq(ostm->irq, ostm_timer_interrupt,
> > +			  IRQF_TIMER | IRQF_IRQPOLL | IRQF_NOBALANCING,
> > +			  dev_name(&ostm->pdev->dev), ostm);
> 
> 	devm_request_irq
> 
> Are you sure the IRQF_NOBALANCING flag should be used ? By default the
> interrupt is pinned to cpu0 below. If this timer is used as a broadcast
> timer for a system with deep idle, you may want to add the DYNIRQ flag
> feature to improve the wakeups on the system.

OK, thank you. After some reading, I understand IRQF_NOBALANCING better.

You're point is valid: Regardless of the fact that the SoCs I'll be using
this on are all single core, like you said below the cpu mask is set to cpu0.

I will remove IRQF_NOBALANCING.


> > +static int __init ostm_probe(struct platform_device *pdev) {
> > +	struct ostm_device *ostm;
> > +	struct resource *res;
> > +	int ret = -EFAULT;
> > +
> > +	if (!is_early_platform_device(pdev)) {
> > +		pm_runtime_set_active(&pdev->dev);
> > +		pm_runtime_enable(&pdev->dev);
> > +	}
> 
> Can you clarify why the 'early' is needed here ?

Actually, it means nothing.

I was using sh_mtu2.c and sh_cmt.c as reference and they were registering
an "earlytimer" which made me think the kernel would probe this driver
early in boot....but....now I see that is just a SH4 kernel specific thing.

So, I will remove all the early platform reference stuff.

Of course I still have the question of how are you supposed to get a
clocksource up and running early in boot. (but I'll figure that out later).


> I don't see the pm_runtime_get/pm_runtime_put in the corresponding
> function.
> 
> What is the benefit of using pm_runtime in this driver ? Isn't the timer
> supposed to be in an always-on power domain ?

When you register a clocksource, you can specify a .enable and .disable
callback where you can do runtime_pm. However...

A. I'm using clocksource_mmio_init, so no enable/dispable option is
   possible.
B. I just found out today that runtime pm is not going to work well on the
   SoC this driver was intended for, so it pointless anyway.

I'll remove all the runtime pm stuff.
Thank you for point this out.

> > +	ostm = platform_get_drvdata(pdev);
> > +	if (ostm) {
> > +		dev_info(&pdev->dev, "kept as earlytimer\n");
> > +		ret = 0;
> > +		goto out;
> > +	}
> > +
> > +	ostm = kzalloc(sizeof(*ostm), GFP_KERNEL);
> 
> 	devm_kzalloc ?

OK, thank you.
Means I can also get rid of '#include <linux/slab.h>'

> > +	if (!ostm) {
> > +		dev_err(&pdev->dev, "failed to allocate memory\n");
> 
> A memory failure allocation always triggers a dumpstack (IIRC), so this
> error message is pointless.

OK, removed.


> > +		return -ENOMEM;
> > +	}
> > +
> > +	ostm->pdev = pdev;
> > +	platform_set_drvdata(ostm->pdev, ostm);
> > +
> > +	res = platform_get_resource(ostm->pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(&ostm->pdev->dev, "failed to get I/O memory\n");
> > +		goto err;
> > +	}
> > +
> > +	ostm->base = ioremap_nocache(res->start, resource_size(res));
> 
> 	devm_ioremap_nocache ?

OK, thank you.
Just need to also add '#include <linux/io.h>'

(looks like devm_ioremap and devm_ioremap_nocache is not as popular as
devm_kzalloc for some reason)


> > +	/* First probed device will be used as system clocksource */
> > +	if (!system_clock) {
> > +		/* use as clocksource */
> > +		ret = ostm_init_clksrc(ostm);
> > +
> > +		/* use as system scheduling clock */
> > +		if (!ret)
> > +			ret = ostm_init_sched_clock(ostm);
> > +
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "failed to use as sched_clock\n");
> > +			system_clock = (void *)-1; /* prevent future attempts */
> > +			ret = 0;	/* still works as clocksource */
> > +		}
> 
> This error code check is unnecessary complex. ostm_init_sched_clock always
> return zero.

Good point.
And since sched_clock_register() always returns void, ostm_init_sched_clock
might was well return void as well.


> > +err:
> > +	if (ret) {
> > +		if (ostm->clk)
> > +			clk_disable_unprepare(ostm->clk);
> > +		if (ostm->base)
> > +			iounmap(ostm->base);
> > +		kfree(ostm);
> > +		platform_set_drvdata(pdev, NULL);
> 
> As iomap, kzalloc were done with the devm, then the rollback will be
> handled with the device framework.

I didn't realize that. Thank you.


> > +static int __init ostm_init(void)
> > +{
> > +	return platform_driver_register(&ostm_timer);
> > +}
> > +
> > +static void __exit ostm_exit(void)
> > +{
> > +	platform_driver_unregister(&ostm_timer);
> > +}
> > +
> > +early_platform_init("earlytimer", &ostm_timer);
> > +subsys_initcall(ostm_init); module_exit(ostm_exit);
> > +
> > +MODULE_AUTHOR("Chris Brandt");
> > +MODULE_DESCRIPTION("Renesas OSTM Timer Driver"); MODULE_LICENSE("GPL
> > +v2");
> 
> Maybe you can try with builtin_platform ?

Good idea. But, now I get a "Section mismatch" during link time so I'll
have to figure out why that is.



Thank you,
Chris

Re: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Daniel Lezcano]

On Tue, Jan 24, 2017 at 04:45:47AM +0000, Chris Brandt wrote:

Hi Chris,

[ ... ]

> > > +	bool "Renesas OSTM timer driver" if COMPILE_TEST
> > > +	depends on GENERIC_CLOCKEVENTS
> > > +	select CLKSRC_MMIO
> > > +	default SYS_SUPPORTS_RENESAS_OSTM
> > 
> > - default SYS_SUPPORTS_RENESAS_OSTM
> > 
> > Except I missing something, I don' the point of having this intermediate
> > config option.
> 
> I was basically following what all the other clocksource drivers do.
> Selecting an ARCH will then set SYS_SUPPORTS_xxTIMERxx which will then
> force xxTIMERxx=y.

Yeah, little by little these options are clean up to be more consistent across
the different drivers. The sh/mtu drivers are different from the other drivers.

The idea with the config option is to let the platform to select the drivers,
and optionnaly allows the compilation on other architecture to increase the
compilation test coverage. That is the reason of COMPILE_TEST.

> But, if "COMPILE_TEST" is set, you can choose what clocksource timers
> you want to build in.
> 
> Is your suggestion to do away with the COMPILE_TEST ability and
> just force RENESAS_OSTM=y if ARCH_R7S72100 is selected?

Just like that:

config RENESAS_OSTM
	bool "Renesas OSTM timer driver" if COMPILE_TEST
	depends on GENERIC_CLOCKEVENTS
	select CLKSRC_MMIO
	help
	  Enables the support for the Renesas OSTM

And then from arch/arm/mach-shmobile/Kconfig:

	select RENESAS_OSTM


> > > +#include <linux/module.h>
> > 
> > Please remove everything module related here and below. This driver is not
> > supposed to be removed and it is compiled in with the Kconfig option.
> 
> Good point. I will remove.
> 
> I guess if you can't compile it as a module, you don't need things like
> 'MODULE_LICENSE'.

Right.

[ ... ]

> > > +static int __init ostm_init_clksrc(struct ostm_device *ostm) {
> > > +	int ret;
> > > +
> > > +	/* irq not used (clock sources don't use interrupts) */
> > > +
> > > +	/* stop counter */
> > 
> > One line comment is usually in the network code. Please use multiline.
> > 
> > /*
> >  * Bla bla
> >  */
> 
> I'm confused. Do you mean:
> 
> A) use multiline formatting for every single-line comment throughout
>    the file?
> 
> B) use multiline for any place where you have 2 or more single-line
>    comments back to back?

I think it is simpler if you just ignore my comment, remove all your comments
in this function, implement a couple of wrapper (eg. clksrc_stop, clksrc_start)
which will speak by themselves.

By the way, is it normal stopping the clockevent is the same code than stopping
the clocksource ?

[ ... ]

> > > +	if (!is_early_platform_device(pdev)) {
> > > +		pm_runtime_set_active(&pdev->dev);
> > > +		pm_runtime_enable(&pdev->dev);
> > > +	}
> > 
> > Can you clarify why the 'early' is needed here ?
> 
> Actually, it means nothing.
> 
> I was using sh_mtu2.c and sh_cmt.c as reference and they were registering
> an "earlytimer" which made me think the kernel would probe this driver
> early in boot....but....now I see that is just a SH4 kernel specific thing.
> 
> So, I will remove all the early platform reference stuff.
> 
> Of course I still have the question of how are you supposed to get a
> clocksource up and running early in boot. (but I'll figure that out later).

Until the hardware clocksource is registered, the jiffies are used as source of
time. That happens at the very early boot time. The clockevent must be
registered early to update the jiffies but you won't have to care about that if
you use the macro below.

[ ... ]

> > > +early_platform_init("earlytimer", &ostm_timer);
> > > +subsys_initcall(ostm_init); module_exit(ostm_exit);
> > > +
> > > +MODULE_AUTHOR("Chris Brandt");
> > > +MODULE_DESCRIPTION("Renesas OSTM Timer Driver"); MODULE_LICENSE("GPL
> > > +v2");
> > 
> > Maybe you can try with builtin_platform ?
> 
> Good idea. But, now I get a "Section mismatch" during link time so I'll
> have to figure out why that is.

Mmh, I think it would be more consistent to convert this to:

CLOCKSOURCE_OF_DECLARE(ostm, "renesas,ostm", ostm_init);

The only problem is to get the struct device associated to the of_node passed
as parameter to ostm_init in order to use the devm_* API.

I think of_find_device_by_node should return the platform_device, then
pdev->dev. If that works the other drivers will benefit from that to remove all
the rollback code everywhere.

  -- Daniel

-- 

 <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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Daniel Lezcano]

On Tue, Jan 24, 2017 at 04:45:47AM +0000, Chris Brandt wrote:

Hi Chris,

[ ... ]

> > > +	bool "Renesas OSTM timer driver" if COMPILE_TEST
> > > +	depends on GENERIC_CLOCKEVENTS
> > > +	select CLKSRC_MMIO
> > > +	default SYS_SUPPORTS_RENESAS_OSTM
> > 
> > - default SYS_SUPPORTS_RENESAS_OSTM
> > 
> > Except I missing something, I don' the point of having this intermediate
> > config option.
> 
> I was basically following what all the other clocksource drivers do.
> Selecting an ARCH will then set SYS_SUPPORTS_xxTIMERxx which will then
> force xxTIMERxx=y.

Yeah, little by little these options are clean up to be more consistent across
the different drivers. The sh/mtu drivers are different from the other drivers.

The idea with the config option is to let the platform to select the drivers,
and optionnaly allows the compilation on other architecture to increase the
compilation test coverage. That is the reason of COMPILE_TEST.

> But, if "COMPILE_TEST" is set, you can choose what clocksource timers
> you want to build in.
> 
> Is your suggestion to do away with the COMPILE_TEST ability and
> just force RENESAS_OSTM=y if ARCH_R7S72100 is selected?

Just like that:

config RENESAS_OSTM
	bool "Renesas OSTM timer driver" if COMPILE_TEST
	depends on GENERIC_CLOCKEVENTS
	select CLKSRC_MMIO
	help
	  Enables the support for the Renesas OSTM

And then from arch/arm/mach-shmobile/Kconfig:

	select RENESAS_OSTM


> > > +#include <linux/module.h>
> > 
> > Please remove everything module related here and below. This driver is not
> > supposed to be removed and it is compiled in with the Kconfig option.
> 
> Good point. I will remove.
> 
> I guess if you can't compile it as a module, you don't need things like
> 'MODULE_LICENSE'.

Right.

[ ... ]

> > > +static int __init ostm_init_clksrc(struct ostm_device *ostm) {
> > > +	int ret;
> > > +
> > > +	/* irq not used (clock sources don't use interrupts) */
> > > +
> > > +	/* stop counter */
> > 
> > One line comment is usually in the network code. Please use multiline.
> > 
> > /*
> >  * Bla bla
> >  */
> 
> I'm confused. Do you mean:
> 
> A) use multiline formatting for every single-line comment throughout
>    the file?
> 
> B) use multiline for any place where you have 2 or more single-line
>    comments back to back?

I think it is simpler if you just ignore my comment, remove all your comments
in this function, implement a couple of wrapper (eg. clksrc_stop, clksrc_start)
which will speak by themselves.

By the way, is it normal stopping the clockevent is the same code than stopping
the clocksource ?

[ ... ]

> > > +	if (!is_early_platform_device(pdev)) {
> > > +		pm_runtime_set_active(&pdev->dev);
> > > +		pm_runtime_enable(&pdev->dev);
> > > +	}
> > 
> > Can you clarify why the 'early' is needed here ?
> 
> Actually, it means nothing.
> 
> I was using sh_mtu2.c and sh_cmt.c as reference and they were registering
> an "earlytimer" which made me think the kernel would probe this driver
> early in boot....but....now I see that is just a SH4 kernel specific thing.
> 
> So, I will remove all the early platform reference stuff.
> 
> Of course I still have the question of how are you supposed to get a
> clocksource up and running early in boot. (but I'll figure that out later).

Until the hardware clocksource is registered, the jiffies are used as source of
time. That happens at the very early boot time. The clockevent must be
registered early to update the jiffies but you won't have to care about that if
you use the macro below.

[ ... ]

> > > +early_platform_init("earlytimer", &ostm_timer);
> > > +subsys_initcall(ostm_init); module_exit(ostm_exit);
> > > +
> > > +MODULE_AUTHOR("Chris Brandt");
> > > +MODULE_DESCRIPTION("Renesas OSTM Timer Driver"); MODULE_LICENSE("GPL
> > > +v2");
> > 
> > Maybe you can try with builtin_platform ?
> 
> Good idea. But, now I get a "Section mismatch" during link time so I'll
> have to figure out why that is.

Mmh, I think it would be more consistent to convert this to:

CLOCKSOURCE_OF_DECLARE(ostm, "renesas,ostm", ostm_init);

The only problem is to get the struct device associated to the of_node passed
as parameter to ostm_init in order to use the devm_* API.

I think of_find_device_by_node should return the platform_device, then
pdev->dev. If that works the other drivers will benefit from that to remove all
the rollback code everywhere.

  -- Daniel

-- 

 <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

RE: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Chris Brandt]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1961 bytes --]

Hi Daniel,

On Tuesday, January 24, 2017, Daniel Lezcano wrote:
> > > > +early_platform_init("earlytimer", &ostm_timer);
> > > > +subsys_initcall(ostm_init); module_exit(ostm_exit);
> > > > +
> > > > +MODULE_AUTHOR("Chris Brandt");
> > > > +MODULE_DESCRIPTION("Renesas OSTM Timer Driver");
> > > > +MODULE_LICENSE("GPL v2");
> > >
> > > Maybe you can try with builtin_platform ?
> >
> > Good idea. But, now I get a "Section mismatch" during link time so
> > I'll have to figure out why that is.
> 
> Mmh, I think it would be more consistent to convert this to:
> 
> CLOCKSOURCE_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
> 
> The only problem is to get the struct device associated to the of_node
> passed as parameter to ostm_init in order to use the devm_* API.
> 
> I think of_find_device_by_node should return the platform_device, then
> pdev->dev. If that works the other drivers will benefit from that to
> pdev->remove all
> the rollback code everywhere.


So I realized that in order to use builtin_platform, I can't have any of the
functions in __init because the build system has no idea that I never plan
on removing or probing again after boot. But, even if I take out all the
__init from my code, I'm still calling clocksource_mmio_init which is __init
so I can never escape the "Section mismatch".

This leaves me with 2 options:

1) Pull the clocksource_mmio driver code into my driver and leave everything
   out of __init
2) Rewrite as a DT driver using CLOCKSOURCE_OF_DECLARE which puts most of
   the code in __init

Of course the better (lower system memory) answer is #2.

I'll try your trick about of_find_device_by_node because while there are
equivalent OF functions for mapping resources, I do like the auto-rollback
feature of demv

Thank you,
Chris
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

RE: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Chris Brandt]

Hi Daniel,

On Tuesday, January 24, 2017, Daniel Lezcano wrote:
> > > > +early_platform_init("earlytimer", &ostm_timer);
> > > > +subsys_initcall(ostm_init); module_exit(ostm_exit);
> > > > +
> > > > +MODULE_AUTHOR("Chris Brandt");
> > > > +MODULE_DESCRIPTION("Renesas OSTM Timer Driver");
> > > > +MODULE_LICENSE("GPL v2");
> > >
> > > Maybe you can try with builtin_platform ?
> >
> > Good idea. But, now I get a "Section mismatch" during link time so
> > I'll have to figure out why that is.
> 
> Mmh, I think it would be more consistent to convert this to:
> 
> CLOCKSOURCE_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
> 
> The only problem is to get the struct device associated to the of_node
> passed as parameter to ostm_init in order to use the devm_* API.
> 
> I think of_find_device_by_node should return the platform_device, then
> pdev->dev. If that works the other drivers will benefit from that to
> pdev->remove all
> the rollback code everywhere.


So I realized that in order to use builtin_platform, I can't have any of the
functions in __init because the build system has no idea that I never plan
on removing or probing again after boot. But, even if I take out all the
__init from my code, I'm still calling clocksource_mmio_init which is __init
so I can never escape the "Section mismatch".

This leaves me with 2 options:

1) Pull the clocksource_mmio driver code into my driver and leave everything
   out of __init
2) Rewrite as a DT driver using CLOCKSOURCE_OF_DECLARE which puts most of
   the code in __init

Of course the better (lower system memory) answer is #2.

I'll try your trick about of_find_device_by_node because while there are
equivalent OF functions for mapping resources, I do like the auto-rollback
feature of demv

Thank you,
Chris

Re: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Geert Uytterhoeven]

Hi Chris,

On Tue, Jan 24, 2017 at 3:43 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tuesday, January 24, 2017, Daniel Lezcano wrote:
>> > > > +early_platform_init("earlytimer", &ostm_timer);
>> > > > +subsys_initcall(ostm_init); module_exit(ostm_exit);
>> > > > +
>> > > > +MODULE_AUTHOR("Chris Brandt");
>> > > > +MODULE_DESCRIPTION("Renesas OSTM Timer Driver");
>> > > > +MODULE_LICENSE("GPL v2");
>> > >
>> > > Maybe you can try with builtin_platform ?
>> >
>> > Good idea. But, now I get a "Section mismatch" during link time so
>> > I'll have to figure out why that is.
>>
>> Mmh, I think it would be more consistent to convert this to:
>>
>> CLOCKSOURCE_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
>>
>> The only problem is to get the struct device associated to the of_node
>> passed as parameter to ostm_init in order to use the devm_* API.
>>
>> I think of_find_device_by_node should return the platform_device, then
>> pdev->dev. If that works the other drivers will benefit from that to
>> pdev->remove all
>> the rollback code everywhere.
>
> So I realized that in order to use builtin_platform, I can't have any of the
> functions in __init because the build system has no idea that I never plan
> on removing or probing again after boot. But, even if I take out all the
> __init from my code, I'm still calling clocksource_mmio_init which is __init
> so I can never escape the "Section mismatch".

For single-probe drivers, you can use builtin_platform_driver_probe().

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Chris Brandt]

Hi Geert,

On Wednesday, January 25, 2017, Geert Uytterhoeven wrote:
> > So I realized that in order to use builtin_platform, I can't have any
> > of the functions in __init because the build system has no idea that I
> > never plan on removing or probing again after boot. But, even if I
> > take out all the __init from my code, I'm still calling
> > clocksource_mmio_init which is __init so I can never escape the "Section
> mismatch".
> 
> For single-probe drivers, you can use builtin_platform_driver_probe().


Thank you for the info.


Chris

RE: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Chris Brandt]

Hi Daniel,

On Tuesday, January 24, 2017, Daniel Lezcano wrote:
> > > > +early_platform_init("earlytimer", &ostm_timer);
> > > > +subsys_initcall(ostm_init); module_exit(ostm_exit);
> > > > +
> > > > +MODULE_AUTHOR("Chris Brandt");
> > > > +MODULE_DESCRIPTION("Renesas OSTM Timer Driver");
> > > > +MODULE_LICENSE("GPL v2");
> > >
> > > Maybe you can try with builtin_platform ?
> >
> > Good idea. But, now I get a "Section mismatch" during link time so
> > I'll have to figure out why that is.
> 
> Mmh, I think it would be more consistent to convert this to:
> 
> CLOCKSOURCE_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
> 
> The only problem is to get the struct device associated to the of_node
> passed as parameter to ostm_init in order to use the devm_* API.
> 
> I think of_find_device_by_node should return the platform_device, then
> pdev->dev. If that works the other drivers will benefit from that to
> pdev->remove all
> the rollback code everywhere.


It was a good idea....but it will not work.

While CLOCKSOURCE_OF_DECLARE is good at putting the driver at the
front of the line for loading, it's too early in boot to detect
a platform_device.

of_find_device_by_node calls bus_find_device. But the first thing that
bus_find_device does is:

	if (!bus || !bus->p)
		return NULL;

But bus->p=0 so it returns immediately.


Of course changing the code over to:
        devm_kzalloc -> devm_kzalloc
devm_ioremap_nocache -> of_io_request_and_map
    platform_get_irq -> irq_of_parse_and_map
             dev_err -> pr_err


Then things work, but I'm back to managing the rollback code manually.


Any other ideas on how to get the corresponding platform_device for
a DT node?


Chris

RE: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Chris Brandt]

Hi Daniel,

On Tuesday, January 24, 2017, Daniel Lezcano wrote:
> > > > +early_platform_init("earlytimer", &ostm_timer);
> > > > +subsys_initcall(ostm_init); module_exit(ostm_exit);
> > > > +
> > > > +MODULE_AUTHOR("Chris Brandt");
> > > > +MODULE_DESCRIPTION("Renesas OSTM Timer Driver");
> > > > +MODULE_LICENSE("GPL v2");
> > >
> > > Maybe you can try with builtin_platform ?
> >
> > Good idea. But, now I get a "Section mismatch" during link time so
> > I'll have to figure out why that is.
> 
> Mmh, I think it would be more consistent to convert this to:
> 
> CLOCKSOURCE_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
> 
> The only problem is to get the struct device associated to the of_node
> passed as parameter to ostm_init in order to use the devm_* API.
> 
> I think of_find_device_by_node should return the platform_device, then
> pdev->dev. If that works the other drivers will benefit from that to
> pdev->remove all
> the rollback code everywhere.


It was a good idea....but it will not work.

While CLOCKSOURCE_OF_DECLARE is good at putting the driver at the
front of the line for loading, it's too early in boot to detect
a platform_device.

of_find_device_by_node calls bus_find_device. But the first thing that
bus_find_device does is:

	if (!bus || !bus->p)
		return NULL;

But bus->p=0 so it returns immediately.


Of course changing the code over to:
        devm_kzalloc -> devm_kzalloc
devm_ioremap_nocache -> of_io_request_and_map
    platform_get_irq -> irq_of_parse_and_map
             dev_err -> pr_err


Then things work, but I'm back to managing the rollback code manually.


Any other ideas on how to get the corresponding platform_device for
a DT node?


Chris

Re: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Geert Uytterhoeven]

On Tue, Jan 24, 2017 at 9:19 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tuesday, January 24, 2017, Daniel Lezcano wrote:
>> > > > +early_platform_init("earlytimer", &ostm_timer);
>> > > > +subsys_initcall(ostm_init); module_exit(ostm_exit);
>> > > > +
>> > > > +MODULE_AUTHOR("Chris Brandt");
>> > > > +MODULE_DESCRIPTION("Renesas OSTM Timer Driver");
>> > > > +MODULE_LICENSE("GPL v2");
>> > >
>> > > Maybe you can try with builtin_platform ?
>> >
>> > Good idea. But, now I get a "Section mismatch" during link time so
>> > I'll have to figure out why that is.
>>
>> Mmh, I think it would be more consistent to convert this to:
>>
>> CLOCKSOURCE_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
>>
>> The only problem is to get the struct device associated to the of_node
>> passed as parameter to ostm_init in order to use the devm_* API.
>>
>> I think of_find_device_by_node should return the platform_device, then
>> pdev->dev. If that works the other drivers will benefit from that to
>> pdev->remove all
>> the rollback code everywhere.
>
> It was a good idea....but it will not work.
>
> While CLOCKSOURCE_OF_DECLARE is good at putting the driver at the
> front of the line for loading, it's too early in boot to detect
> a platform_device.

That's correct. All those *_OF_DECLARE() style initializations start
to break as soon as power and/or clock domains are involved.
That's one reason why some subsystems (e.g. clock) are moving away from it.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Daniel Lezcano]

On Tue, Jan 24, 2017 at 08:19:50PM +0000, Chris Brandt wrote:
> Hi Daniel,
> 
> On Tuesday, January 24, 2017, Daniel Lezcano wrote:
> > > > > +early_platform_init("earlytimer", &ostm_timer);
> > > > > +subsys_initcall(ostm_init); module_exit(ostm_exit);
> > > > > +
> > > > > +MODULE_AUTHOR("Chris Brandt");
> > > > > +MODULE_DESCRIPTION("Renesas OSTM Timer Driver");
> > > > > +MODULE_LICENSE("GPL v2");
> > > >
> > > > Maybe you can try with builtin_platform ?
> > >
> > > Good idea. But, now I get a "Section mismatch" during link time so
> > > I'll have to figure out why that is.
> > 
> > Mmh, I think it would be more consistent to convert this to:
> > 
> > CLOCKSOURCE_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
> > 
> > The only problem is to get the struct device associated to the of_node
> > passed as parameter to ostm_init in order to use the devm_* API.
> > 
> > I think of_find_device_by_node should return the platform_device, then
> > pdev->dev. If that works the other drivers will benefit from that to
> > pdev->remove all
> > the rollback code everywhere.
> 
> 
> It was a good idea....but it will not work.
> 
> While CLOCKSOURCE_OF_DECLARE is good at putting the driver at the
> front of the line for loading, it's too early in boot to detect
> a platform_device.
> 
> of_find_device_by_node calls bus_find_device. But the first thing that
> bus_find_device does is:
> 
> 	if (!bus || !bus->p)
> 		return NULL;
> 
> But bus->p=0 so it returns immediately.
> 
> 
> Of course changing the code over to:
>         devm_kzalloc -> devm_kzalloc
> devm_ioremap_nocache -> of_io_request_and_map
>     platform_get_irq -> irq_of_parse_and_map
>              dev_err -> pr_err
> 
> 
> Then things work, but I'm back to managing the rollback code manually.
> 
> 
> Any other ideas on how to get the corresponding platform_device for
> a DT node?

No :/

So up to you.
	- CLOCKSOURCE_OF_DECLARE consistent but need rollback
	or
	- platform_device but with another timer available at early time

-- 

 <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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Daniel Lezcano]

On Tue, Jan 24, 2017 at 08:19:50PM +0000, Chris Brandt wrote:
> Hi Daniel,
> 
> On Tuesday, January 24, 2017, Daniel Lezcano wrote:
> > > > > +early_platform_init("earlytimer", &ostm_timer);
> > > > > +subsys_initcall(ostm_init); module_exit(ostm_exit);
> > > > > +
> > > > > +MODULE_AUTHOR("Chris Brandt");
> > > > > +MODULE_DESCRIPTION("Renesas OSTM Timer Driver");
> > > > > +MODULE_LICENSE("GPL v2");
> > > >
> > > > Maybe you can try with builtin_platform ?
> > >
> > > Good idea. But, now I get a "Section mismatch" during link time so
> > > I'll have to figure out why that is.
> > 
> > Mmh, I think it would be more consistent to convert this to:
> > 
> > CLOCKSOURCE_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
> > 
> > The only problem is to get the struct device associated to the of_node
> > passed as parameter to ostm_init in order to use the devm_* API.
> > 
> > I think of_find_device_by_node should return the platform_device, then
> > pdev->dev. If that works the other drivers will benefit from that to
> > pdev->remove all
> > the rollback code everywhere.
> 
> 
> It was a good idea....but it will not work.
> 
> While CLOCKSOURCE_OF_DECLARE is good at putting the driver at the
> front of the line for loading, it's too early in boot to detect
> a platform_device.
> 
> of_find_device_by_node calls bus_find_device. But the first thing that
> bus_find_device does is:
> 
> 	if (!bus || !bus->p)
> 		return NULL;
> 
> But bus->p=0 so it returns immediately.
> 
> 
> Of course changing the code over to:
>         devm_kzalloc -> devm_kzalloc
> devm_ioremap_nocache -> of_io_request_and_map
>     platform_get_irq -> irq_of_parse_and_map
>              dev_err -> pr_err
> 
> 
> Then things work, but I'm back to managing the rollback code manually.
> 
> 
> Any other ideas on how to get the corresponding platform_device for
> a DT node?

No :/

So up to you.
	- CLOCKSOURCE_OF_DECLARE consistent but need rollback
	or
	- platform_device but with another timer available at early time

-- 

 <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

RE: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

[Chris Brandt]

Hi Daniel,

On Wednesday, January 25, 2017, Daniel Lezcano wrote:
> > Then things work, but I'm back to managing the rollback code manually.
> >
> >
> > Any other ideas on how to get the corresponding platform_device for a
> > DT node?
> 
> No :/
> 
> So up to you.
> 	- CLOCKSOURCE_OF_DECLARE consistent but need rollback
> 	or
> 	- platform_device but with another timer available at early time

As far as I can tell, the rollback functions don't mind if I pass NULL
pointers to them. So with CLOCKSOURCE_OF_DECLARE, my error rollback at
the end of ostm_init is basically:


err:
	if (ret) {
		clk_disable_unprepare(ostm_clk);
		iounmap(ostm->base);
		kfree(ostm);
		return ret;
	}

	return 0;
}



If I go with CLOCKSOURCE_OF_DECLARE, I can at least get rid of the early boot message
"clocksource_probe: no matching clocksources found"

I'll go ahead and send a v4 today with all the changes you suggested.

Thank you for your help.

Regards,
Chris