[PATCH v2 0/2] clocksource: npcm: add NPCM7xx timer driver

[PATCH v2 0/2] clocksource: npcm: add NPCM7xx timer driver

[Tomer Maimon]

Addressed comments from:
 - Daniel Lezcano: https://www.spinics.net/lists/devicetree/msg196683.html

Changes since version 1:
 - Rename driver name
 - Removing unnecessary dependencies in configuration
 - Adding prefix to the macros
 - No changes to dt-binding documentation since v1

Asked Brendon to modify NPCM7xx device tree for support NPCM7xx timer driver
https://www.spinics.net/lists/arm-kernel/msg614424.html

Changes have been tested on the NPCM750 evaluation board.

Tomer Maimon (2):
  clocksource: npcm: add NPCM7xx timer driver
  dt-binding: timer: document NPCM7xx timer DT bindings

 .../bindings/timer/nuvoton,npcm7xx-timer.txt       |  25 +++
 drivers/clocksource/Kconfig                        |   8 +
 drivers/clocksource/Makefile                       |   1 +
 drivers/clocksource/npcm7xx-timer.c                | 224 +++++++++++++++++++++
 4 files changed, 258 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/nuvoton,npcm7xx-timer.txt
 create mode 100644 drivers/clocksource/npcm7xx-timer.c

-- 
2.14.1

[PATCH v2 1/2] clocksource: npcm: add NPCM7xx timer driver

[Tomer Maimon]

Add Nuvoton BMC NPCM7xx timer driver.

the clocksource Enable 24-bit TIMER0 and TIMER1 counters,
while TIMER0 serves as clockevent and TIMER1 serves as clocksource.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 drivers/clocksource/Kconfig         |   8 ++
 drivers/clocksource/Makefile        |   1 +
 drivers/clocksource/npcm7xx-timer.c | 224 ++++++++++++++++++++++++++++++++++++
 3 files changed, 233 insertions(+)
 create mode 100644 drivers/clocksource/npcm7xx-timer.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index cc6062049170..46184af75d6c 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -140,6 +140,14 @@ config VT8500_TIMER
 	help
 	  Enables support for the VT8500 driver.
 
+config NPCM7XX_TIMER
+	bool "NPCM7xx timer driver" if COMPILE_TEST
+	depends on HAS_IOMEM
+	select CLKSRC_MMIO
+	help
+	  Enable 24-bit TIMER0 and TIMER1 counters in the NPCM7xx arcithcture,
+	  While TIMER0 serves as clockevent and TIMER1 serves as clocksource.
+
 config CADENCE_TTC_TIMER
 	bool "Cadence TTC timer driver" if COMPILE_TEST
 	depends on COMMON_CLK
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index dbc1ad14515e..f2a9318a1eec 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_CLKSRC_TI_32K)	+= timer-ti-32k.o
 obj-$(CONFIG_CLKSRC_NPS)	+= timer-nps.o
 obj-$(CONFIG_OXNAS_RPS_TIMER)	+= timer-oxnas-rps.o
 obj-$(CONFIG_OWL_TIMER)		+= owl-timer.o
+obj-$(CONFIG_NPCM7XX_TIMER)	+= npcm7xx-timer.o
 
 obj-$(CONFIG_ARC_TIMERS)		+= arc_timer.o
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
diff --git a/drivers/clocksource/npcm7xx-timer.c b/drivers/clocksource/npcm7xx-timer.c
new file mode 100644
index 000000000000..d1ea5c183a69
--- /dev/null
+++ b/drivers/clocksource/npcm7xx-timer.c
@@ -0,0 +1,224 @@
+
+/*
+ * Copyright (c) 2017 Nuvoton Technology corporation.
+ * Copyright 2017 Google, Inc.
+ *
+ * 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;version 2 of the License.
+ */
+
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/clockchips.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+
+struct npcm7xx_clockevent_data {
+	struct clock_event_device cvd;
+	void __iomem *timer_base;
+	unsigned int rate;
+};
+
+/* Timers registers */
+#define NPCM7XX_REG_TCSR0	0x0 /* Timer 0 Control and Status Register */
+#define NPCM7XX_REG_TICR0	0x8 /* Timer 0 Initial Count Register */
+#define NPCM7XX_REG_TCSR1	0x4 /* Timer 1 Control and Status Register */
+#define NPCM7XX_REG_TICR1	0xc /* Timer 1 Initial Count Register */
+#define NPCM7XX_REG_TDR1	0x14 /* Timer 1 Data Register */
+#define NPCM7XX_REG_TISR	0x18 /* Timer Interrupt Status Register */
+
+/* Timers control */
+#define NPCM7XX_Tx_RESETINT		0x1f
+#define NPCM7XX_Tx_PERIOD		BIT(27)
+#define NPCM7XX_Tx_INTEN		BIT(29)
+#define NPCM7XX_Tx_COUNTEN		BIT(30)
+#define NPCM7XX_Tx_ONESHOT		0x0
+#define NPCM7XX_Tx_OPER			GENMASK(3, 27)
+#define NPCM7XX_Tx_MIN_PRESCALE		0x1
+#define NPCM7XX_Tx_TDR_MASK_BITS	24
+#define NPCM7XX_Tx_MAX_CNT		0xFFFFFF
+#define NPCM7XX_T0_CLR_INT		0x1
+#define NPCM7XX_Tx_CLR_CSR		0x0
+
+/* Timers operating mode */
+#define NPCM7XX_START_PERIODIC_Tx (NPCM7XX_Tx_PERIOD | NPCM7XX_Tx_COUNTEN | \
+					NPCM7XX_Tx_INTEN | \
+					NPCM7XX_Tx_MIN_PRESCALE)
+
+#define NPCM7XX_START_ONESHOT_Tx (NPCM7XX_Tx_ONESHOT | NPCM7XX_Tx_COUNTEN | \
+					NPCM7XX_Tx_INTEN | \
+					NPCM7XX_Tx_MIN_PRESCALE)
+#define NPCM7XX_START_Tx (NPCM7XX_Tx_COUNTEN | NPCM7XX_Tx_PERIOD | \
+				NPCM7XX_Tx_MIN_PRESCALE)
+
+static int npcm7xx_timer_oneshot(struct clock_event_device *evt)
+{
+	u32 val;
+	struct npcm7xx_clockevent_data *cevtd =
+		container_of(evt, struct npcm7xx_clockevent_data, cvd);
+
+	val = readl(cevtd->timer_base + NPCM7XX_REG_TCSR0);
+	val &= ~NPCM7XX_Tx_OPER;
+
+	val = readl(cevtd->timer_base + NPCM7XX_REG_TCSR0);
+	val |= NPCM7XX_START_ONESHOT_Tx;
+	writel(val, cevtd->timer_base + NPCM7XX_REG_TCSR0);
+
+	return 0;
+}
+
+static int npcm7xx_timer_periodic(struct clock_event_device *evt)
+{
+	struct npcm7xx_clockevent_data *cevtd =
+		container_of(evt, struct npcm7xx_clockevent_data, cvd);
+	u32 val;
+
+	val = readl(cevtd->timer_base + NPCM7XX_REG_TCSR0);
+	val &= ~NPCM7XX_Tx_OPER;
+
+	writel((cevtd->rate / HZ), cevtd->timer_base + NPCM7XX_REG_TICR0);
+	val |= NPCM7XX_START_PERIODIC_Tx;
+
+	writel(val, cevtd->timer_base + NPCM7XX_REG_TCSR0);
+
+	return 0;
+}
+
+static int npcm7xx_clockevent_setnextevent(unsigned long evt,
+		struct clock_event_device *clk)
+{
+	struct npcm7xx_clockevent_data *cevtd =
+		container_of(clk, struct npcm7xx_clockevent_data, cvd);
+	u32 val;
+
+	writel(evt, cevtd->timer_base + NPCM7XX_REG_TICR0);
+	val = readl(cevtd->timer_base + NPCM7XX_REG_TCSR0);
+	val |= NPCM7XX_START_Tx;
+	writel(val, cevtd->timer_base + NPCM7XX_REG_TCSR0);
+
+	return 0;
+}
+
+static struct npcm7xx_clockevent_data npcm7xx_clockevent_data = {
+	.cvd = {
+		.name		    = "npcm7xx-timer0",
+		.features	    = CLOCK_EVT_FEAT_PERIODIC |
+				      CLOCK_EVT_FEAT_ONESHOT,
+		.set_next_event	    = npcm7xx_clockevent_setnextevent,
+		.set_state_shutdown = npcm7xx_timer_oneshot,
+		.set_state_periodic = npcm7xx_timer_periodic,
+		.set_state_oneshot  = npcm7xx_timer_oneshot,
+		.tick_resume	    = npcm7xx_timer_oneshot,
+		.rating		    = 300
+	},
+};
+
+static irqreturn_t npcm7xx_timer0_interrupt(int irq, void *dev_id)
+{
+	struct npcm7xx_clockevent_data *cevtd = dev_id;
+	struct clock_event_device *evt = &cevtd->cvd;
+
+	writel(NPCM7XX_T0_CLR_INT, cevtd->timer_base + NPCM7XX_REG_TISR);
+
+	if (evt->event_handler) {
+		evt->event_handler(evt);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static struct irqaction npcm7xx_timer0_irq = {
+	.name		= "npcm7xx-timer0",
+	.flags		= IRQF_TIMER | IRQF_IRQPOLL,
+	.handler	= npcm7xx_timer0_interrupt,
+	.dev_id		= &npcm7xx_clockevent_data,
+};
+
+static void __init npcm7xx_clockevents_init(int irq, u32 rate)
+{
+	writel(NPCM7XX_Tx_CLR_CSR,
+		npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TCSR0);
+
+	writel(NPCM7XX_Tx_RESETINT,
+		npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TISR);
+	setup_irq(irq, &npcm7xx_timer0_irq);
+	npcm7xx_clockevent_data.cvd.cpumask = cpumask_of(0);
+
+	clockevents_config_and_register(&npcm7xx_clockevent_data.cvd, rate,
+					0x1, NPCM7XX_Tx_MAX_CNT);
+}
+
+static void __init npcm7xx_clocksource_init(u32 rate)
+{
+	u32 val;
+
+	writel(NPCM7XX_Tx_CLR_CSR,
+		npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TCSR1);
+	writel(NPCM7XX_Tx_MAX_CNT,
+		npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TICR1);
+
+	val = readl(npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TCSR1);
+	val |= NPCM7XX_START_Tx;
+	writel(val, npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TCSR1);
+
+	clocksource_mmio_init(npcm7xx_clockevent_data.timer_base +
+				NPCM7XX_REG_TDR1,
+				"npcm7xx-timer1", rate,
+				300, (unsigned int)NPCM7XX_Tx_TDR_MASK_BITS,
+				clocksource_mmio_readl_down);
+}
+
+static int __init npcm7xx_timer_init(struct device_node *np)
+{
+	struct clk *clk;
+	int irq, ret;
+	u32 rate;
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (!irq)
+		return -EINVAL;
+
+	npcm7xx_clockevent_data.timer_base = of_iomap(np, 0);
+	if (!npcm7xx_clockevent_data.timer_base)
+		return -ENXIO;
+
+	clk = of_clk_get(np, 0);
+
+	if (IS_ERR(clk)) {
+		ret = of_property_read_u32(np, "clock-frequency", &rate);
+		if (ret)
+			goto err_iounmap;
+	} else {
+		clk_prepare_enable(clk);
+		rate = clk_get_rate(clk);
+	}
+
+	/* Clock input is divided by PRESCALE + 1 before it is fed */
+	/* to the counter */
+	rate = rate / (NPCM7XX_Tx_MIN_PRESCALE + 1);
+
+	npcm7xx_clockevent_data.rate = rate;
+
+	npcm7xx_clocksource_init(rate);
+	npcm7xx_clockevents_init(irq, rate);
+
+	pr_info("Enabling NPCM7xx clocksource timer base: %p, IRQ: %u\n",
+		npcm7xx_clockevent_data.timer_base, irq);
+
+	return 0;
+
+err_iounmap:
+	iounmap(npcm7xx_clockevent_data.timer_base);
+	return ret;
+
+}
+
+TIMER_OF_DECLARE(npcm7xx, "nuvoton,npcm7xx-timer", npcm7xx_timer_init);
+
-- 
2.14.1

[PATCH v2 2/2] dt-binding: timer: document NPCM7xx timer DT bindings

[Tomer Maimon]

Added device tree binding documentation for Nuvoton NPCM7xx timer.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 .../bindings/timer/nuvoton,npcm7xx-timer.txt       | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/nuvoton,npcm7xx-timer.txt

diff --git a/Documentation/devicetree/bindings/timer/nuvoton,npcm7xx-timer.txt b/Documentation/devicetree/bindings/timer/nuvoton,npcm7xx-timer.txt
new file mode 100644
index 000000000000..d1beea901509
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/nuvoton,npcm7xx-timer.txt
@@ -0,0 +1,25 @@
+Nuvoton NPCM7xx timer
+
+Nuvoton NPCM7xx have three timer modules, each timer module provides five 24-bit
+timer counters.
+
+Required properties:
+- compatible      : "nuvoton,npcm7xx-timer" for Poleg NPCM7xx.
+- reg             : Offset and length of the register set for the device.
+- interrupts      : Contain the timer interrupt with flags for
+                    falling edge.
+
+Required clocking property, have to be one of:
+- clocks          : phandle of timer reference clock.
+- clock-frequency : The frequency in Hz of the clock that drives the NPCM7xx
+                    timer (usually 25000000).
+
+Example:
+
+timer@f0008000 {
+    compatible = "nuvoton,npcm7xx-timer";
+    interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+    reg = <0xf0008000 0x1000>;
+    clocks = <&clk NPCM7XX_CLK_TIMER>;
+};
+
-- 
2.14.1

Re: [PATCH v2 0/2] clocksource: npcm: add NPCM7xx timer driver

[Brendan Higgins]

On Wed, Nov 1, 2017 at 5:16 AM, Tomer Maimon <tmaimon77@gmail.com> wrote:
> Addressed comments from:
>  - Daniel Lezcano: https://www.spinics.net/lists/devicetree/msg196683.html
>
> Changes since version 1:
>  - Rename driver name
>  - Removing unnecessary dependencies in configuration
>  - Adding prefix to the macros
>  - No changes to dt-binding documentation since v1
>
> Asked Brendon to modify NPCM7xx device tree for support NPCM7xx timer driver
> https://www.spinics.net/lists/arm-kernel/msg614424.html

Rob Herring asks that we keep the device tree binding the same as I have it.
Unless, there is some part of the conversation that I am missing.

>
> Changes have been tested on the NPCM750 evaluation board.
>
> Tomer Maimon (2):
>   clocksource: npcm: add NPCM7xx timer driver
>   dt-binding: timer: document NPCM7xx timer DT bindings
>
>  .../bindings/timer/nuvoton,npcm7xx-timer.txt       |  25 +++
>  drivers/clocksource/Kconfig                        |   8 +
>  drivers/clocksource/Makefile                       |   1 +
>  drivers/clocksource/npcm7xx-timer.c                | 224 +++++++++++++++++++++
>  4 files changed, 258 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/nuvoton,npcm7xx-timer.txt
>  create mode 100644 drivers/clocksource/npcm7xx-timer.c
>
> --
> 2.14.1
>

Re: [PATCH v2 1/2] clocksource: npcm: add NPCM7xx timer driver

[Brendan Higgins]

Looks like you missed a couple of comments from Daniel:
https://www.spinics.net/lists/devicetree/msg196683.html

Also, I think the binding documentation is supposed to come before the driver.

On Wed, Nov 1, 2017 at 5:16 AM, Tomer Maimon <tmaimon77@gmail.com> wrote:
> Add Nuvoton BMC NPCM7xx timer driver.
>
> the clocksource Enable 24-bit TIMER0 and TIMER1 counters,
> while TIMER0 serves as clockevent and TIMER1 serves as clocksource.
>
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
>  drivers/clocksource/Kconfig         |   8 ++
>  drivers/clocksource/Makefile        |   1 +
>  drivers/clocksource/npcm7xx-timer.c | 224 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 233 insertions(+)
>  create mode 100644 drivers/clocksource/npcm7xx-timer.c

Looks like you forgot to rename the driver.

>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index cc6062049170..46184af75d6c 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -140,6 +140,14 @@ config VT8500_TIMER
>         help
>           Enables support for the VT8500 driver.
>
> +config NPCM7XX_TIMER
> +       bool "NPCM7xx timer driver" if COMPILE_TEST
> +       depends on HAS_IOMEM
> +       select CLKSRC_MMIO
> +       help
> +         Enable 24-bit TIMER0 and TIMER1 counters in the NPCM7xx arcithcture,
> +         While TIMER0 serves as clockevent and TIMER1 serves as clocksource.
> +
>  config CADENCE_TTC_TIMER
>         bool "Cadence TTC timer driver" if COMPILE_TEST
>         depends on COMMON_CLK
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index dbc1ad14515e..f2a9318a1eec 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_CLKSRC_TI_32K)   += timer-ti-32k.o
>  obj-$(CONFIG_CLKSRC_NPS)       += timer-nps.o
>  obj-$(CONFIG_OXNAS_RPS_TIMER)  += timer-oxnas-rps.o
>  obj-$(CONFIG_OWL_TIMER)                += owl-timer.o
> +obj-$(CONFIG_NPCM7XX_TIMER)    += npcm7xx-timer.o
>
>  obj-$(CONFIG_ARC_TIMERS)               += arc_timer.o
>  obj-$(CONFIG_ARM_ARCH_TIMER)           += arm_arch_timer.o
> diff --git a/drivers/clocksource/npcm7xx-timer.c b/drivers/clocksource/npcm7xx-timer.c
> new file mode 100644
> index 000000000000..d1ea5c183a69
> --- /dev/null
> +++ b/drivers/clocksource/npcm7xx-timer.c
> @@ -0,0 +1,224 @@
> +
> +/*
> + * Copyright (c) 2017 Nuvoton Technology corporation.
> + * Copyright 2017 Google, Inc.
> + *
> + * 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;version 2 of the License.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/clockchips.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +
> +struct npcm7xx_clockevent_data {
> +       struct clock_event_device cvd;
> +       void __iomem *timer_base;
> +       unsigned int rate;
> +};
> +
> +/* Timers registers */
> +#define NPCM7XX_REG_TCSR0      0x0 /* Timer 0 Control and Status Register */
> +#define NPCM7XX_REG_TICR0      0x8 /* Timer 0 Initial Count Register */
> +#define NPCM7XX_REG_TCSR1      0x4 /* Timer 1 Control and Status Register */
> +#define NPCM7XX_REG_TICR1      0xc /* Timer 1 Initial Count Register */
> +#define NPCM7XX_REG_TDR1       0x14 /* Timer 1 Data Register */
> +#define NPCM7XX_REG_TISR       0x18 /* Timer Interrupt Status Register */
> +
> +/* Timers control */
> +#define NPCM7XX_Tx_RESETINT            0x1f
> +#define NPCM7XX_Tx_PERIOD              BIT(27)
> +#define NPCM7XX_Tx_INTEN               BIT(29)
> +#define NPCM7XX_Tx_COUNTEN             BIT(30)
> +#define NPCM7XX_Tx_ONESHOT             0x0
> +#define NPCM7XX_Tx_OPER                        GENMASK(3, 27)
> +#define NPCM7XX_Tx_MIN_PRESCALE                0x1
> +#define NPCM7XX_Tx_TDR_MASK_BITS       24
> +#define NPCM7XX_Tx_MAX_CNT             0xFFFFFF
> +#define NPCM7XX_T0_CLR_INT             0x1
> +#define NPCM7XX_Tx_CLR_CSR             0x0
> +
> +/* Timers operating mode */
> +#define NPCM7XX_START_PERIODIC_Tx (NPCM7XX_Tx_PERIOD | NPCM7XX_Tx_COUNTEN | \
> +                                       NPCM7XX_Tx_INTEN | \
> +                                       NPCM7XX_Tx_MIN_PRESCALE)
> +
> +#define NPCM7XX_START_ONESHOT_Tx (NPCM7XX_Tx_ONESHOT | NPCM7XX_Tx_COUNTEN | \
> +                                       NPCM7XX_Tx_INTEN | \
> +                                       NPCM7XX_Tx_MIN_PRESCALE)
> +#define NPCM7XX_START_Tx (NPCM7XX_Tx_COUNTEN | NPCM7XX_Tx_PERIOD | \
> +                               NPCM7XX_Tx_MIN_PRESCALE)
> +
> +static int npcm7xx_timer_oneshot(struct clock_event_device *evt)
> +{
> +       u32 val;
> +       struct npcm7xx_clockevent_data *cevtd =
> +               container_of(evt, struct npcm7xx_clockevent_data, cvd);
> +
> +       val = readl(cevtd->timer_base + NPCM7XX_REG_TCSR0);
> +       val &= ~NPCM7XX_Tx_OPER;
> +
> +       val = readl(cevtd->timer_base + NPCM7XX_REG_TCSR0);
> +       val |= NPCM7XX_START_ONESHOT_Tx;
> +       writel(val, cevtd->timer_base + NPCM7XX_REG_TCSR0);
> +
> +       return 0;
> +}
> +
> +static int npcm7xx_timer_periodic(struct clock_event_device *evt)
> +{
> +       struct npcm7xx_clockevent_data *cevtd =
> +               container_of(evt, struct npcm7xx_clockevent_data, cvd);
> +       u32 val;
> +
> +       val = readl(cevtd->timer_base + NPCM7XX_REG_TCSR0);
> +       val &= ~NPCM7XX_Tx_OPER;
> +
> +       writel((cevtd->rate / HZ), cevtd->timer_base + NPCM7XX_REG_TICR0);
> +       val |= NPCM7XX_START_PERIODIC_Tx;
> +
> +       writel(val, cevtd->timer_base + NPCM7XX_REG_TCSR0);
> +
> +       return 0;
> +}
> +
> +static int npcm7xx_clockevent_setnextevent(unsigned long evt,
> +               struct clock_event_device *clk)
> +{
> +       struct npcm7xx_clockevent_data *cevtd =
> +               container_of(clk, struct npcm7xx_clockevent_data, cvd);
> +       u32 val;
> +
> +       writel(evt, cevtd->timer_base + NPCM7XX_REG_TICR0);
> +       val = readl(cevtd->timer_base + NPCM7XX_REG_TCSR0);
> +       val |= NPCM7XX_START_Tx;
> +       writel(val, cevtd->timer_base + NPCM7XX_REG_TCSR0);
> +
> +       return 0;
> +}
> +
> +static struct npcm7xx_clockevent_data npcm7xx_clockevent_data = {
> +       .cvd = {
> +               .name               = "npcm7xx-timer0",
> +               .features           = CLOCK_EVT_FEAT_PERIODIC |
> +                                     CLOCK_EVT_FEAT_ONESHOT,
> +               .set_next_event     = npcm7xx_clockevent_setnextevent,
> +               .set_state_shutdown = npcm7xx_timer_oneshot,
> +               .set_state_periodic = npcm7xx_timer_periodic,
> +               .set_state_oneshot  = npcm7xx_timer_oneshot,
> +               .tick_resume        = npcm7xx_timer_oneshot,
> +               .rating             = 300
> +       },
> +};
> +
> +static irqreturn_t npcm7xx_timer0_interrupt(int irq, void *dev_id)
> +{
> +       struct npcm7xx_clockevent_data *cevtd = dev_id;
> +       struct clock_event_device *evt = &cevtd->cvd;
> +
> +       writel(NPCM7XX_T0_CLR_INT, cevtd->timer_base + NPCM7XX_REG_TISR);
> +
> +       if (evt->event_handler) {

I think Daniel was implying this is impossible or at least deserves a comment.
I think I originally added this check to make the driver not crash when I was
ripping out some of the HAL dependencies. I did not think about it too hard, so
this might need some looking into.

> +               evt->event_handler(evt);
> +               return IRQ_HANDLED;
> +       }
> +
> +       return IRQ_NONE;
> +}
> +
> +static struct irqaction npcm7xx_timer0_irq = {
> +       .name           = "npcm7xx-timer0",
> +       .flags          = IRQF_TIMER | IRQF_IRQPOLL,
> +       .handler        = npcm7xx_timer0_interrupt,
> +       .dev_id         = &npcm7xx_clockevent_data,
> +};
> +
> +static void __init npcm7xx_clockevents_init(int irq, u32 rate)
> +{
> +       writel(NPCM7XX_Tx_CLR_CSR,
> +               npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TCSR0);
> +
> +       writel(NPCM7XX_Tx_RESETINT,
> +               npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TISR);
> +       setup_irq(irq, &npcm7xx_timer0_irq);
> +       npcm7xx_clockevent_data.cvd.cpumask = cpumask_of(0);
> +
> +       clockevents_config_and_register(&npcm7xx_clockevent_data.cvd, rate,
> +                                       0x1, NPCM7XX_Tx_MAX_CNT);
> +}
> +
> +static void __init npcm7xx_clocksource_init(u32 rate)
> +{
> +       u32 val;
> +
> +       writel(NPCM7XX_Tx_CLR_CSR,
> +               npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TCSR1);
> +       writel(NPCM7XX_Tx_MAX_CNT,
> +               npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TICR1);
> +
> +       val = readl(npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TCSR1);
> +       val |= NPCM7XX_START_Tx;
> +       writel(val, npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TCSR1);
> +
> +       clocksource_mmio_init(npcm7xx_clockevent_data.timer_base +
> +                               NPCM7XX_REG_TDR1,
> +                               "npcm7xx-timer1", rate,
> +                               300, (unsigned int)NPCM7XX_Tx_TDR_MASK_BITS,
> +                               clocksource_mmio_readl_down);
> +}
> +
> +static int __init npcm7xx_timer_init(struct device_node *np)
> +{
> +       struct clk *clk;
> +       int irq, ret;
> +       u32 rate;
> +
> +       irq = irq_of_parse_and_map(np, 0);

Looks like this, the base addr, and possibly the clock is common with what
timer_of_init does.

> +       if (!irq)
> +               return -EINVAL;
> +
> +       npcm7xx_clockevent_data.timer_base = of_iomap(np, 0);
> +       if (!npcm7xx_clockevent_data.timer_base)
> +               return -ENXIO;
> +
> +       clk = of_clk_get(np, 0);
> +
> +       if (IS_ERR(clk)) {
> +               ret = of_property_read_u32(np, "clock-frequency", &rate);
> +               if (ret)
> +                       goto err_iounmap;
> +       } else {
> +               clk_prepare_enable(clk);
> +               rate = clk_get_rate(clk);
> +       }
> +
> +       /* Clock input is divided by PRESCALE + 1 before it is fed */
> +       /* to the counter */
> +       rate = rate / (NPCM7XX_Tx_MIN_PRESCALE + 1);
> +
> +       npcm7xx_clockevent_data.rate = rate;
> +
> +       npcm7xx_clocksource_init(rate);
> +       npcm7xx_clockevents_init(irq, rate);
> +
> +       pr_info("Enabling NPCM7xx clocksource timer base: %p, IRQ: %u\n",
> +               npcm7xx_clockevent_data.timer_base, irq);
> +
> +       return 0;
> +
> +err_iounmap:
> +       iounmap(npcm7xx_clockevent_data.timer_base);
> +       return ret;
> +
> +}
> +
> +TIMER_OF_DECLARE(npcm7xx, "nuvoton,npcm7xx-timer", npcm7xx_timer_init);
> +
> --
> 2.14.1
>

Re: [PATCH v2 0/2] clocksource: npcm: add NPCM7xx timer driver

[Tomer Maimon]

On 4 November 2017 at 02:28, Brendan Higgins <brendanhiggins@google.com> wrote:
> On Wed, Nov 1, 2017 at 5:16 AM, Tomer Maimon <tmaimon77@gmail.com> wrote:
>> Addressed comments from:
>>  - Daniel Lezcano: https://www.spinics.net/lists/devicetree/msg196683.html
>>
>> Changes since version 1:
>>  - Rename driver name
>>  - Removing unnecessary dependencies in configuration
>>  - Adding prefix to the macros
>>  - No changes to dt-binding documentation since v1
>>
>> Asked Brendon to modify NPCM7xx device tree for support NPCM7xx timer driver
>> https://www.spinics.net/lists/arm-kernel/msg614424.html
>
> Rob Herring asks that we keep the device tree binding the same as I have it.
> Unless, there is some part of the conversation that I am missing.

you are right that he asked to keep the competible as you have in the
device tree but
the reg is incorrect in your device tree, please modify it as follow:

timer0: timer@f0008000 {
        compatible = "nuvoton,npcm750-timer";
        interrupts = <0 32 4>;
        reg = <0xf0008000 0x1000>;
        clocks = <&clk NPCM7XX_CLK_TIMER>;
};

>
>>
>> Changes have been tested on the NPCM750 evaluation board.
>>
>> Tomer Maimon (2):
>>   clocksource: npcm: add NPCM7xx timer driver
>>   dt-binding: timer: document NPCM7xx timer DT bindings
>>
>>  .../bindings/timer/nuvoton,npcm7xx-timer.txt       |  25 +++
>>  drivers/clocksource/Kconfig                        |   8 +
>>  drivers/clocksource/Makefile                       |   1 +
>>  drivers/clocksource/npcm7xx-timer.c                | 224 +++++++++++++++++++++
>>  4 files changed, 258 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/timer/nuvoton,npcm7xx-timer.txt
>>  create mode 100644 drivers/clocksource/npcm7xx-timer.c
>>
>> --
>> 2.14.1
>>

Thanks!

Re: [PATCH v2 1/2] clocksource: npcm: add NPCM7xx timer driver

[Tomer Maimon]

On 4 November 2017 at 02:28, Brendan Higgins <brendanhiggins@google.com> wrote:
> Looks like you missed a couple of comments from Daniel:
> https://www.spinics.net/lists/devicetree/msg196683.html
>
> Also, I think the binding documentation is supposed to come before the driver.
>
> On Wed, Nov 1, 2017 at 5:16 AM, Tomer Maimon <tmaimon77@gmail.com> wrote:
>> Add Nuvoton BMC NPCM7xx timer driver.
>>
>> the clocksource Enable 24-bit TIMER0 and TIMER1 counters,
>> while TIMER0 serves as clockevent and TIMER1 serves as clocksource.
>>
>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
>> ---
>>  drivers/clocksource/Kconfig         |   8 ++
>>  drivers/clocksource/Makefile        |   1 +
>>  drivers/clocksource/npcm7xx-timer.c | 224 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 233 insertions(+)
>>  create mode 100644 drivers/clocksource/npcm7xx-timer.c
>
> Looks like you forgot to rename the driver.

I think Daniel asked to modify the driver name from npcm7xx_timer.c
to npcm7xx-timer.c and i did it.
>
>>
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index cc6062049170..46184af75d6c 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -140,6 +140,14 @@ config VT8500_TIMER
>>         help
>>           Enables support for the VT8500 driver.
>>
>> +config NPCM7XX_TIMER
>> +       bool "NPCM7xx timer driver" if COMPILE_TEST
>> +       depends on HAS_IOMEM
>> +       select CLKSRC_MMIO
>> +       help
>> +         Enable 24-bit TIMER0 and TIMER1 counters in the NPCM7xx arcithcture,
>> +         While TIMER0 serves as clockevent and TIMER1 serves as clocksource.
>> +
>>  config CADENCE_TTC_TIMER
>>         bool "Cadence TTC timer driver" if COMPILE_TEST
>>         depends on COMMON_CLK
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> index dbc1ad14515e..f2a9318a1eec 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -53,6 +53,7 @@ obj-$(CONFIG_CLKSRC_TI_32K)   += timer-ti-32k.o
>>  obj-$(CONFIG_CLKSRC_NPS)       += timer-nps.o
>>  obj-$(CONFIG_OXNAS_RPS_TIMER)  += timer-oxnas-rps.o
>>  obj-$(CONFIG_OWL_TIMER)                += owl-timer.o
>> +obj-$(CONFIG_NPCM7XX_TIMER)    += npcm7xx-timer.o
>>
>>  obj-$(CONFIG_ARC_TIMERS)               += arc_timer.o
>>  obj-$(CONFIG_ARM_ARCH_TIMER)           += arm_arch_timer.o
>> diff --git a/drivers/clocksource/npcm7xx-timer.c b/drivers/clocksource/npcm7xx-timer.c
>> new file mode 100644
>> index 000000000000..d1ea5c183a69
>> --- /dev/null
>> +++ b/drivers/clocksource/npcm7xx-timer.c
>> @@ -0,0 +1,224 @@
>> +
>> +/*
>> + * Copyright (c) 2017 Nuvoton Technology corporation.
>> + * Copyright 2017 Google, Inc.
>> + *
>> + * 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;version 2 of the License.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/sched.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/err.h>
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/clockchips.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> +
>> +struct npcm7xx_clockevent_data {
>> +       struct clock_event_device cvd;
>> +       void __iomem *timer_base;
>> +       unsigned int rate;
>> +};
>> +
>> +/* Timers registers */
>> +#define NPCM7XX_REG_TCSR0      0x0 /* Timer 0 Control and Status Register */
>> +#define NPCM7XX_REG_TICR0      0x8 /* Timer 0 Initial Count Register */
>> +#define NPCM7XX_REG_TCSR1      0x4 /* Timer 1 Control and Status Register */
>> +#define NPCM7XX_REG_TICR1      0xc /* Timer 1 Initial Count Register */
>> +#define NPCM7XX_REG_TDR1       0x14 /* Timer 1 Data Register */
>> +#define NPCM7XX_REG_TISR       0x18 /* Timer Interrupt Status Register */
>> +
>> +/* Timers control */
>> +#define NPCM7XX_Tx_RESETINT            0x1f
>> +#define NPCM7XX_Tx_PERIOD              BIT(27)
>> +#define NPCM7XX_Tx_INTEN               BIT(29)
>> +#define NPCM7XX_Tx_COUNTEN             BIT(30)
>> +#define NPCM7XX_Tx_ONESHOT             0x0
>> +#define NPCM7XX_Tx_OPER                        GENMASK(3, 27)
>> +#define NPCM7XX_Tx_MIN_PRESCALE                0x1
>> +#define NPCM7XX_Tx_TDR_MASK_BITS       24
>> +#define NPCM7XX_Tx_MAX_CNT             0xFFFFFF
>> +#define NPCM7XX_T0_CLR_INT             0x1
>> +#define NPCM7XX_Tx_CLR_CSR             0x0
>> +
>> +/* Timers operating mode */
>> +#define NPCM7XX_START_PERIODIC_Tx (NPCM7XX_Tx_PERIOD | NPCM7XX_Tx_COUNTEN | \
>> +                                       NPCM7XX_Tx_INTEN | \
>> +                                       NPCM7XX_Tx_MIN_PRESCALE)
>> +
>> +#define NPCM7XX_START_ONESHOT_Tx (NPCM7XX_Tx_ONESHOT | NPCM7XX_Tx_COUNTEN | \
>> +                                       NPCM7XX_Tx_INTEN | \
>> +                                       NPCM7XX_Tx_MIN_PRESCALE)
>> +#define NPCM7XX_START_Tx (NPCM7XX_Tx_COUNTEN | NPCM7XX_Tx_PERIOD | \
>> +                               NPCM7XX_Tx_MIN_PRESCALE)
>> +
>> +static int npcm7xx_timer_oneshot(struct clock_event_device *evt)
>> +{
>> +       u32 val;
>> +       struct npcm7xx_clockevent_data *cevtd =
>> +               container_of(evt, struct npcm7xx_clockevent_data, cvd);
>> +
>> +       val = readl(cevtd->timer_base + NPCM7XX_REG_TCSR0);
>> +       val &= ~NPCM7XX_Tx_OPER;
>> +
>> +       val = readl(cevtd->timer_base + NPCM7XX_REG_TCSR0);
>> +       val |= NPCM7XX_START_ONESHOT_Tx;
>> +       writel(val, cevtd->timer_base + NPCM7XX_REG_TCSR0);
>> +
>> +       return 0;
>> +}
>> +
>> +static int npcm7xx_timer_periodic(struct clock_event_device *evt)
>> +{
>> +       struct npcm7xx_clockevent_data *cevtd =
>> +               container_of(evt, struct npcm7xx_clockevent_data, cvd);
>> +       u32 val;
>> +
>> +       val = readl(cevtd->timer_base + NPCM7XX_REG_TCSR0);
>> +       val &= ~NPCM7XX_Tx_OPER;
>> +
>> +       writel((cevtd->rate / HZ), cevtd->timer_base + NPCM7XX_REG_TICR0);
>> +       val |= NPCM7XX_START_PERIODIC_Tx;
>> +
>> +       writel(val, cevtd->timer_base + NPCM7XX_REG_TCSR0);
>> +
>> +       return 0;
>> +}
>> +
>> +static int npcm7xx_clockevent_setnextevent(unsigned long evt,
>> +               struct clock_event_device *clk)
>> +{
>> +       struct npcm7xx_clockevent_data *cevtd =
>> +               container_of(clk, struct npcm7xx_clockevent_data, cvd);
>> +       u32 val;
>> +
>> +       writel(evt, cevtd->timer_base + NPCM7XX_REG_TICR0);
>> +       val = readl(cevtd->timer_base + NPCM7XX_REG_TCSR0);
>> +       val |= NPCM7XX_START_Tx;
>> +       writel(val, cevtd->timer_base + NPCM7XX_REG_TCSR0);
>> +
>> +       return 0;
>> +}
>> +
>> +static struct npcm7xx_clockevent_data npcm7xx_clockevent_data = {
>> +       .cvd = {
>> +               .name               = "npcm7xx-timer0",
>> +               .features           = CLOCK_EVT_FEAT_PERIODIC |
>> +                                     CLOCK_EVT_FEAT_ONESHOT,
>> +               .set_next_event     = npcm7xx_clockevent_setnextevent,
>> +               .set_state_shutdown = npcm7xx_timer_oneshot,
>> +               .set_state_periodic = npcm7xx_timer_periodic,
>> +               .set_state_oneshot  = npcm7xx_timer_oneshot,
>> +               .tick_resume        = npcm7xx_timer_oneshot,
>> +               .rating             = 300
>> +       },
>> +};
>> +
>> +static irqreturn_t npcm7xx_timer0_interrupt(int irq, void *dev_id)
>> +{
>> +       struct npcm7xx_clockevent_data *cevtd = dev_id;
>> +       struct clock_event_device *evt = &cevtd->cvd;
>> +
>> +       writel(NPCM7XX_T0_CLR_INT, cevtd->timer_base + NPCM7XX_REG_TISR);
>> +
>> +       if (evt->event_handler) {
>
> I think Daniel was implying this is impossible or at least deserves a comment.
> I think I originally added this check to make the driver not crash when I was
> ripping out some of the HAL dependencies. I did not think about it too hard, so
> this might need some looking into.

I found the issue, calling incorrect procedure in set_state_shutdown().
will send a fix in V3.
>
>> +               evt->event_handler(evt);
>> +               return IRQ_HANDLED;
>> +       }
>> +
>> +       return IRQ_NONE;
>> +}
>> +
>> +static struct irqaction npcm7xx_timer0_irq = {
>> +       .name           = "npcm7xx-timer0",
>> +       .flags          = IRQF_TIMER | IRQF_IRQPOLL,
>> +       .handler        = npcm7xx_timer0_interrupt,
>> +       .dev_id         = &npcm7xx_clockevent_data,
>> +};
>> +
>> +static void __init npcm7xx_clockevents_init(int irq, u32 rate)
>> +{
>> +       writel(NPCM7XX_Tx_CLR_CSR,
>> +               npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TCSR0);
>> +
>> +       writel(NPCM7XX_Tx_RESETINT,
>> +               npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TISR);
>> +       setup_irq(irq, &npcm7xx_timer0_irq);
>> +       npcm7xx_clockevent_data.cvd.cpumask = cpumask_of(0);
>> +
>> +       clockevents_config_and_register(&npcm7xx_clockevent_data.cvd, rate,
>> +                                       0x1, NPCM7XX_Tx_MAX_CNT);
>> +}
>> +
>> +static void __init npcm7xx_clocksource_init(u32 rate)
>> +{
>> +       u32 val;
>> +
>> +       writel(NPCM7XX_Tx_CLR_CSR,
>> +               npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TCSR1);
>> +       writel(NPCM7XX_Tx_MAX_CNT,
>> +               npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TICR1);
>> +
>> +       val = readl(npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TCSR1);
>> +       val |= NPCM7XX_START_Tx;
>> +       writel(val, npcm7xx_clockevent_data.timer_base + NPCM7XX_REG_TCSR1);
>> +
>> +       clocksource_mmio_init(npcm7xx_clockevent_data.timer_base +
>> +                               NPCM7XX_REG_TDR1,
>> +                               "npcm7xx-timer1", rate,
>> +                               300, (unsigned int)NPCM7XX_Tx_TDR_MASK_BITS,
>> +                               clocksource_mmio_readl_down);
>> +}
>> +
>> +static int __init npcm7xx_timer_init(struct device_node *np)
>> +{
>> +       struct clk *clk;
>> +       int irq, ret;
>> +       u32 rate;
>> +
>> +       irq = irq_of_parse_and_map(np, 0);
>
> Looks like this, the base addr, and possibly the clock is common with what
> timer_of_init does.
see above.
>
>> +       if (!irq)
>> +               return -EINVAL;
>> +
>> +       npcm7xx_clockevent_data.timer_base = of_iomap(np, 0);
>> +       if (!npcm7xx_clockevent_data.timer_base)
>> +               return -ENXIO;
>> +
>> +       clk = of_clk_get(np, 0);
>> +
>> +       if (IS_ERR(clk)) {
>> +               ret = of_property_read_u32(np, "clock-frequency", &rate);
>> +               if (ret)
>> +                       goto err_iounmap;
>> +       } else {
>> +               clk_prepare_enable(clk);
>> +               rate = clk_get_rate(clk);
>> +       }
>> +
>> +       /* Clock input is divided by PRESCALE + 1 before it is fed */
>> +       /* to the counter */
>> +       rate = rate / (NPCM7XX_Tx_MIN_PRESCALE + 1);
>> +
>> +       npcm7xx_clockevent_data.rate = rate;
>> +
>> +       npcm7xx_clocksource_init(rate);
>> +       npcm7xx_clockevents_init(irq, rate);
>> +
>> +       pr_info("Enabling NPCM7xx clocksource timer base: %p, IRQ: %u\n",
>> +               npcm7xx_clockevent_data.timer_base, irq);
>> +
>> +       return 0;
>> +
>> +err_iounmap:
>> +       iounmap(npcm7xx_clockevent_data.timer_base);
>> +       return ret;
>> +
>> +}
>> +
>> +TIMER_OF_DECLARE(npcm7xx, "nuvoton,npcm7xx-timer", npcm7xx_timer_init);
>> +
>> --
>> 2.14.1
>>

Thanks!