[PATCH] clockevents: rockchip: Add rockchip timer for rk3288

[PATCH] clockevents: rockchip: Add rockchip timer for rk3288

[Daniel Lezcano]

The rk3288 board uses the architected timers and these ones are shutdown when
the cpu is powered down. There is a need of a broadcast timer in this case to
ensure proper wakeup when the cpus are in sleep mode and a timer expires.

This driver provides the basic timer functionnality as a backup for the local
timers at sleep time.

The timer belongs to the alive subsystem. It includes two programmables 64 bits
timer channels but the driver only uses 32bits. It works with two operations
mode: free running and user defined count.

Programing sequence:

1. Timer initialization:
 * Disable the timer by writing '0' to the CONTROLREG register
 * Program the timer mode by writing the mode to the CONTROLREG register
 * Set the interrupt mask

2. Setting the count value:
 * Load the count value to the registers COUNT0 and COUNT1 (not used).

3. Enable the timer
 * Write '1' to the CONTROLREG register with the mode (free running or user)

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 .../bindings/timer/rockchip,rk3288-timer.txt       |  15 ++
 arch/arm/boot/dts/rk3288.dtsi                      |   7 +
 arch/arm/mach-rockchip/Kconfig                     |   1 +
 drivers/clocksource/Kconfig                        |   4 +
 drivers/clocksource/Makefile                       |   1 +
 drivers/clocksource/rockchip_timer.c               | 158 +++++++++++++++++++++
 6 files changed, 186 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/rockchip,rk3288-timer.txt
 create mode 100644 drivers/clocksource/rockchip_timer.c

diff --git a/Documentation/devicetree/bindings/timer/rockchip,rk3288-timer.txt b/Documentation/devicetree/bindings/timer/rockchip,rk3288-timer.txt
new file mode 100644
index 0000000..54ef747
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/rockchip,rk3288-timer.txt
@@ -0,0 +1,15 @@
+Rockchip rk3288 timer
+
+Required properties:
+- compatible: shall be "rockchip,rk3288-timer"
+- reg: base address of the timer register starting with TIMERS CONTROL register
+- interrupts: should contain the interrupts for Timer0
+- clock-frequency: the frequency the timer is running
+
+Example:
+	timer: timer@ff810000 {
+		compatible = "rockchip,rk3288-timer";
+		reg = <0xff810000 0x20>;
+		interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
+		clock-frequency = <24000000>;
+	};
diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 2a878a3..7a7db48 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -149,6 +149,13 @@
 		clock-frequency = <24000000>;
 	};
 
+	timer: timer@ff810000 {
+		compatible = "rockchip,rk3288-timer";
+		reg = <0xff810000 0x20>;
+		interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
+		clock-frequency = <24000000>;
+	};
+
 	sdmmc: dwmmc@ff0c0000 {
 		compatible = "rockchip,rk3288-dw-mshc";
 		clock-freq-min-max = <400000 150000000>;
diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
index ac5803c..4c3fa7e 100644
--- a/arch/arm/mach-rockchip/Kconfig
+++ b/arch/arm/mach-rockchip/Kconfig
@@ -11,6 +11,7 @@ config ARCH_ROCKCHIP
 	select HAVE_ARM_SCU if SMP
 	select HAVE_ARM_TWD if SMP
 	select DW_APB_TIMER_OF
+	select RK3288_TIMER
 	select ARM_GLOBAL_TIMER
 	select CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
 	help
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index fc01ec2..6ed97a6 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -26,6 +26,10 @@ config DW_APB_TIMER_OF
 	select DW_APB_TIMER
 	select CLKSRC_OF
 
+config RK3288_TIMER
+	bool
+	select CLKSRC_OF
+
 config ARMADA_370_XP_TIMER
 	bool
 	select CLKSRC_OF
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 94d90b2..39e4f77 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_CLKBLD_I8253)	+= i8253.o
 obj-$(CONFIG_CLKSRC_MMIO)	+= mmio.o
 obj-$(CONFIG_DW_APB_TIMER)	+= dw_apb_timer.o
 obj-$(CONFIG_DW_APB_TIMER_OF)	+= dw_apb_timer_of.o
+obj-$(CONFIG_RK3288_TIMER)      += rockchip_timer.o
 obj-$(CONFIG_CLKSRC_NOMADIK_MTU)	+= nomadik-mtu.o
 obj-$(CONFIG_CLKSRC_DBX500_PRCMU)	+= clksrc-dbx500-prcmu.o
 obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
new file mode 100644
index 0000000..00e24bd
--- /dev/null
+++ b/drivers/clocksource/rockchip_timer.c
@@ -0,0 +1,158 @@
+/*
+ * Rockchip timer support
+ *
+ * Copyright (C) Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/clockchips.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#define TIMER_NAME "rk_timer"
+
+#define TIMER_LOAD_COUNT0               0x00
+#define TIMER_LOAD_COUNT1               0x04
+#define TIMER_CONTROL_REG               0x10
+#define TIMER_INT_STATUS                0x18
+
+#define TIMER_DISABLE                   0x0
+#define TIMER_ENABLE                    0x1
+#define TIMER_MODE_FREE_RUNNING         (0 << 1)
+#define TIMER_MODE_USER_DEFINED_COUNT   (1 << 1)
+#define TIMER_INT_UNMASK                (1 << 2)
+
+struct bc_timer {
+	struct clock_event_device ce;
+	void __iomem *base;
+	u32 freq;
+};
+
+static struct bc_timer bc_timer;
+
+static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
+{
+	return container_of(ce, struct bc_timer, ce);
+}
+
+static inline void __iomem *rk_base(struct clock_event_device *ce)
+{
+	return rk_timer(ce)->base;
+}
+
+static inline void rk_timer_disable(struct clock_event_device *ce)
+{
+	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
+	dsb();
+}
+
+static inline void rk_timer_enable(struct clock_event_device *ce, u32 flags)
+{
+	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
+		       rk_base(ce) + TIMER_CONTROL_REG);
+	dsb();
+}
+
+static void rk_timer_update_counter(unsigned long cycles,
+				    struct clock_event_device *ce)
+{
+	writel_relaxed(cycles, rk_base(ce) + TIMER_LOAD_COUNT0);
+	writel_relaxed(0, rk_base(ce) + TIMER_LOAD_COUNT1);
+	dsb();
+}
+
+static void rk_timer_interrupt_clear(struct clock_event_device *ce)
+{
+	writel_relaxed(1, rk_base(ce) + TIMER_INT_STATUS);
+	dsb();
+}
+
+static inline int rk_timer_set_next_event(unsigned long cycles,
+					  struct clock_event_device *ce)
+{
+	rk_timer_disable(ce);
+	rk_timer_update_counter(cycles, ce);
+	rk_timer_enable(ce, TIMER_MODE_USER_DEFINED_COUNT);
+	return 0;
+}
+
+static inline void rk_timer_set_mode(enum clock_event_mode mode,
+				     struct clock_event_device *ce)
+{
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+		rk_timer_disable(ce);
+		rk_timer_update_counter(rk_timer(ce)->freq / HZ - 1, ce);
+		rk_timer_enable(ce, TIMER_MODE_FREE_RUNNING);
+	case CLOCK_EVT_MODE_ONESHOT:
+	case CLOCK_EVT_MODE_RESUME:
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+		rk_timer_disable(ce);
+		break;
+	}
+}
+
+static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *ce = dev_id;
+
+	rk_timer_interrupt_clear(ce);
+
+	if (ce->mode == CLOCK_EVT_MODE_ONESHOT) {
+		rk_timer_disable(ce);
+	}
+
+	ce->event_handler(ce);
+
+	return IRQ_HANDLED;
+}
+
+static void __init rk_timer_init(struct device_node *np)
+{
+	struct clock_event_device *ce = &bc_timer.ce;
+	int ret, irq;
+
+	bc_timer.base = of_iomap(np, 0);
+	if (!bc_timer.base) {
+		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
+		return;
+	}
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (irq == NO_IRQ) {
+                pr_err("Failed to map interrupts for '%s'\n", TIMER_NAME);
+                return;
+        }
+
+        if (of_property_read_u32(np, "clock-frequency", &bc_timer.freq)) {
+		pr_err("Failed to read the frequency for '%s'\n", TIMER_NAME);
+		return;
+	}
+
+	ce->name = TIMER_NAME;
+	ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
+	ce->set_next_event = rk_timer_set_next_event;
+	ce->set_mode = rk_timer_set_mode;
+	ce->irq = irq;
+	ce->cpumask = cpumask_of(0);
+	ce->rating = 250;
+
+	rk_timer_interrupt_clear(ce);
+	rk_timer_disable(ce);
+
+	ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce);
+	if (ret) {
+		pr_err("Failed to initialize '%s': %d\n", TIMER_NAME, ret);
+		return;
+	}
+
+	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
+}
+CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
-- 
1.9.1

Re: [PATCH] clockevents: rockchip: Add rockchip timer for rk3288

[Heiko Stübner]

Hi Daniel,

sorry it took a bit longer to reply.

Generally it looks good to me, just some minor issues inline below.

It would also be nice to include the rockchip list (linux-
rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org) for future patches.


Am Dienstag, 6. Januar 2015, 12:03:53 schrieb Daniel Lezcano:
> The rk3288 board uses the architected timers and these ones are shutdown
> when the cpu is powered down. There is a need of a broadcast timer in this
> case to ensure proper wakeup when the cpus are in sleep mode and a timer
> expires.
> 
> This driver provides the basic timer functionnality as a backup for the
> local timers at sleep time.
> 
> The timer belongs to the alive subsystem. It includes two programmables 64
> bits timer channels but the driver only uses 32bits. It works with two
> operations mode: free running and user defined count.
> 
> Programing sequence:
> 
> 1. Timer initialization:
>  * Disable the timer by writing '0' to the CONTROLREG register
>  * Program the timer mode by writing the mode to the CONTROLREG register
>  * Set the interrupt mask
> 
> 2. Setting the count value:
>  * Load the count value to the registers COUNT0 and COUNT1 (not used).
> 
> 3. Enable the timer
>  * Write '1' to the CONTROLREG register with the mode (free running or user)
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  .../bindings/timer/rockchip,rk3288-timer.txt       |  15 ++
>  arch/arm/boot/dts/rk3288.dtsi                      |   7 +
>  arch/arm/mach-rockchip/Kconfig                     |   1 +
>  drivers/clocksource/Kconfig                        |   4 +
>  drivers/clocksource/Makefile                       |   1 +
>  drivers/clocksource/rockchip_timer.c               | 158
> +++++++++++++++++++++ 6 files changed, 186 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/timer/rockchip,rk3288-timer.txt create
> mode 100644 drivers/clocksource/rockchip_timer.c
> 
> diff --git
> a/Documentation/devicetree/bindings/timer/rockchip,rk3288-timer.txt
> b/Documentation/devicetree/bindings/timer/rockchip,rk3288-timer.txt new
> file mode 100644
> index 0000000..54ef747
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/rockchip,rk3288-timer.txt
> @@ -0,0 +1,15 @@
> +Rockchip rk3288 timer
> +
> +Required properties:
> +- compatible: shall be "rockchip,rk3288-timer"
> +- reg: base address of the timer register starting with TIMERS CONTROL
> register +- interrupts: should contain the interrupts for Timer0
> +- clock-frequency: the frequency the timer is running
> +
> +Example:
> +	timer: timer@ff810000 {
> +		compatible = "rockchip,rk3288-timer";
> +		reg = <0xff810000 0x20>;
> +		interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> +		clock-frequency = <24000000>;

wouldn't it make sense to use the actual supplying clock?

For the timer you want to use it is just the non-gateable &xin24m, but the 
timers in the other block (timer0-5) actually do have gateable clocks.

Similarly there is a pclk_timer supplying at least one of the two actual 
blocks. I'll try to inquire how the blocks are actually supplied.


> +	};
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index 2a878a3..7a7db48 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -149,6 +149,13 @@
>  		clock-frequency = <24000000>;
>  	};
> 
> +	timer: timer@ff810000 {
> +		compatible = "rockchip,rk3288-timer";
> +		reg = <0xff810000 0x20>;
> +		interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> +		clock-frequency = <24000000>;
> +	};
> +
>  	sdmmc: dwmmc@ff0c0000 {
>  		compatible = "rockchip,rk3288-dw-mshc";
>  		clock-freq-min-max = <400000 150000000>;
> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> index ac5803c..4c3fa7e 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -11,6 +11,7 @@ config ARCH_ROCKCHIP
>  	select HAVE_ARM_SCU if SMP
>  	select HAVE_ARM_TWD if SMP
>  	select DW_APB_TIMER_OF
> +	select RK3288_TIMER
>  	select ARM_GLOBAL_TIMER
>  	select CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
>  	help
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index fc01ec2..6ed97a6 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -26,6 +26,10 @@ config DW_APB_TIMER_OF
>  	select DW_APB_TIMER
>  	select CLKSRC_OF
> 
> +config RK3288_TIMER
> +	bool
> +	select CLKSRC_OF
> +

the config symbol to compile rockchip_timer.c is RK3288_TIMER?
I'd think it could match a bit more ...
ROCKCHIP_TIMER -> rockchip_timer.c
	or
RK3288_TIMER -> rk3288_timer.c

This timer-block is actually also present on the rk3188, but not on the rk3066 
which uses an unmodified dw_apb_timer.


>  config ARMADA_370_XP_TIMER
>  	bool
>  	select CLKSRC_OF
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 94d90b2..39e4f77 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_CLKBLD_I8253)	+= i8253.o
>  obj-$(CONFIG_CLKSRC_MMIO)	+= mmio.o
>  obj-$(CONFIG_DW_APB_TIMER)	+= dw_apb_timer.o
>  obj-$(CONFIG_DW_APB_TIMER_OF)	+= dw_apb_timer_of.o
> +obj-$(CONFIG_RK3288_TIMER)      += rockchip_timer.o
>  obj-$(CONFIG_CLKSRC_NOMADIK_MTU)	+= nomadik-mtu.o
>  obj-$(CONFIG_CLKSRC_DBX500_PRCMU)	+= clksrc-dbx500-prcmu.o
>  obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
> diff --git a/drivers/clocksource/rockchip_timer.c
> b/drivers/clocksource/rockchip_timer.c new file mode 100644
> index 0000000..00e24bd
> --- /dev/null
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -0,0 +1,158 @@
> +/*
> + * Rockchip timer support
> + *
> + * Copyright (C) Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/clockchips.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
> +#define TIMER_NAME "rk_timer"
> +
> +#define TIMER_LOAD_COUNT0               0x00
> +#define TIMER_LOAD_COUNT1               0x04
> +#define TIMER_CONTROL_REG               0x10
> +#define TIMER_INT_STATUS                0x18
> +
> +#define TIMER_DISABLE                   0x0
> +#define TIMER_ENABLE                    0x1
> +#define TIMER_MODE_FREE_RUNNING         (0 << 1)
> +#define TIMER_MODE_USER_DEFINED_COUNT   (1 << 1)
> +#define TIMER_INT_UNMASK                (1 << 2)
> +
> +struct bc_timer {
> +	struct clock_event_device ce;
> +	void __iomem *base;
> +	u32 freq;
> +};
> +
> +static struct bc_timer bc_timer;
> +
> +static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
> +{
> +	return container_of(ce, struct bc_timer, ce);
> +}
> +
> +static inline void __iomem *rk_base(struct clock_event_device *ce)
> +{
> +	return rk_timer(ce)->base;
> +}
> +
> +static inline void rk_timer_disable(struct clock_event_device *ce)
> +{
> +	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
> +	dsb();
> +}
> +
> +static inline void rk_timer_enable(struct clock_event_device *ce, u32
> flags) +{
> +	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
> +		       rk_base(ce) + TIMER_CONTROL_REG);
> +	dsb();
> +}
> +
> +static void rk_timer_update_counter(unsigned long cycles,
> +				    struct clock_event_device *ce)
> +{
> +	writel_relaxed(cycles, rk_base(ce) + TIMER_LOAD_COUNT0);
> +	writel_relaxed(0, rk_base(ce) + TIMER_LOAD_COUNT1);
> +	dsb();
> +}
> +
> +static void rk_timer_interrupt_clear(struct clock_event_device *ce)
> +{
> +	writel_relaxed(1, rk_base(ce) + TIMER_INT_STATUS);
> +	dsb();
> +}
> +
> +static inline int rk_timer_set_next_event(unsigned long cycles,
> +					  struct clock_event_device *ce)
> +{
> +	rk_timer_disable(ce);
> +	rk_timer_update_counter(cycles, ce);
> +	rk_timer_enable(ce, TIMER_MODE_USER_DEFINED_COUNT);
> +	return 0;
> +}
> +
> +static inline void rk_timer_set_mode(enum clock_event_mode mode,
> +				     struct clock_event_device *ce)
> +{
> +	switch (mode) {
> +	case CLOCK_EVT_MODE_PERIODIC:
> +		rk_timer_disable(ce);
> +		rk_timer_update_counter(rk_timer(ce)->freq / HZ - 1, ce);
> +		rk_timer_enable(ce, TIMER_MODE_FREE_RUNNING);
> +	case CLOCK_EVT_MODE_ONESHOT:
> +	case CLOCK_EVT_MODE_RESUME:
> +		break;
> +	case CLOCK_EVT_MODE_UNUSED:
> +	case CLOCK_EVT_MODE_SHUTDOWN:
> +		rk_timer_disable(ce);
> +		break;
> +	}
> +}
> +
> +static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
> +{
> +	struct clock_event_device *ce = dev_id;
> +
> +	rk_timer_interrupt_clear(ce);
> +
> +	if (ce->mode == CLOCK_EVT_MODE_ONESHOT) {
> +		rk_timer_disable(ce);
> +	}
> +
> +	ce->event_handler(ce);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void __init rk_timer_init(struct device_node *np)
> +{
> +	struct clock_event_device *ce = &bc_timer.ce;
> +	int ret, irq;
> +
> +	bc_timer.base = of_iomap(np, 0);
> +	if (!bc_timer.base) {
> +		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
> +		return;
> +	}
> +
> +	irq = irq_of_parse_and_map(np, 0);
> +	if (irq == NO_IRQ) {
> +                pr_err("Failed to map interrupts for '%s'\n", TIMER_NAME);
> +                return;
> +        }
> +
> +        if (of_property_read_u32(np, "clock-frequency", &bc_timer.freq)) {

formatting issue with spaces instead of tabs in the 5 lines above


Cheers,
Heiko


> +		pr_err("Failed to read the frequency for '%s'\n", TIMER_NAME);
> +		return;
> +	}
> +
> +	ce->name = TIMER_NAME;
> +	ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
> +	ce->set_next_event = rk_timer_set_next_event;
> +	ce->set_mode = rk_timer_set_mode;
> +	ce->irq = irq;
> +	ce->cpumask = cpumask_of(0);
> +	ce->rating = 250;
> +
> +	rk_timer_interrupt_clear(ce);
> +	rk_timer_disable(ce);
> +
> +	ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce);
> +	if (ret) {
> +		pr_err("Failed to initialize '%s': %d\n", TIMER_NAME, ret);
> +		return;
> +	}
> +
> +	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
> +}
> +CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
--
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] clockevents: rockchip: Add rockchip timer for rk3288

[Daniel Lezcano]

On 01/13/2015 12:20 AM, Heiko Stübner wrote:
> Hi Daniel,
>
> sorry it took a bit longer to reply.

No problem :)

> Generally it looks good to me, just some minor issues inline below.

Thanks for the review.

> It would also be nice to include the rockchip list (linux-
> rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org) for future patches.

Yes, sure. I will do that in the future.

[...]

>> +- clock-frequency: the frequency the timer is running
>> +
>> +Example:
>> +	timer: timer@ff810000 {
>> +		compatible = "rockchip,rk3288-timer";
>> +		reg = <0xff810000 0x20>;
>> +		interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
>> +		clock-frequency = <24000000>;
>
> wouldn't it make sense to use the actual supplying clock?
>
> For the timer you want to use it is just the non-gateable &xin24m, but the
> timers in the other block (timer0-5) actually do have gateable clocks.
>
> Similarly there is a pclk_timer supplying at least one of the two actual
> blocks. I'll try to inquire how the blocks are actually supplied.

Ok, are you suggesting I should use another timer so we can gate it for 
power efficiency ?

>> +	};
>> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
>> index 2a878a3..7a7db48 100644
>> --- a/arch/arm/boot/dts/rk3288.dtsi
>> +++ b/arch/arm/boot/dts/rk3288.dtsi
>> @@ -149,6 +149,13 @@
>>   		clock-frequency = <24000000>;
>>   	};
>>
>> +	timer: timer@ff810000 {
>> +		compatible = "rockchip,rk3288-timer";
>> +		reg = <0xff810000 0x20>;
>> +		interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
>> +		clock-frequency = <24000000>;
>> +	};
>> +
>>   	sdmmc: dwmmc@ff0c0000 {
>>   		compatible = "rockchip,rk3288-dw-mshc";
>>   		clock-freq-min-max = <400000 150000000>;
>> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
>> index ac5803c..4c3fa7e 100644
>> --- a/arch/arm/mach-rockchip/Kconfig
>> +++ b/arch/arm/mach-rockchip/Kconfig
>> @@ -11,6 +11,7 @@ config ARCH_ROCKCHIP
>>   	select HAVE_ARM_SCU if SMP
>>   	select HAVE_ARM_TWD if SMP
>>   	select DW_APB_TIMER_OF
>> +	select RK3288_TIMER
>>   	select ARM_GLOBAL_TIMER
>>   	select CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
>>   	help
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index fc01ec2..6ed97a6 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -26,6 +26,10 @@ config DW_APB_TIMER_OF
>>   	select DW_APB_TIMER
>>   	select CLKSRC_OF
>>
>> +config RK3288_TIMER
>> +	bool
>> +	select CLKSRC_OF
>> +
>
> the config symbol to compile rockchip_timer.c is RK3288_TIMER?
> I'd think it could match a bit more ...
> ROCKCHIP_TIMER -> rockchip_timer.c
> 	or
> RK3288_TIMER -> rk3288_timer.c
 >
> This timer-block is actually also present on the rk3188, but not on the rk3066
> which uses an unmodified dw_apb_timer.

Ok, then rockchip_timer.c fits better. I will do the change.

>>   config ARMADA_370_XP_TIMER
>>   	bool
>>   	select CLKSRC_OF
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> +
>> +        if (of_property_read_u32(np, "clock-frequency", &bc_timer.freq)) {
>
> formatting issue with spaces instead of tabs in the 5 lines above

Copy that.

>> +		pr_err("Failed to read the frequency for '%s'\n", TIMER_NAME);
>> +		return;
>> +	}

Thanks!
   -- 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] clockevents: rockchip: Add rockchip timer for rk3288

[Heiko Stübner]

Hi Daniel,

Am Mittwoch, 14. Januar 2015, 17:10:24 schrieb Daniel Lezcano:
> On 01/13/2015 12:20 AM, Heiko Stübner wrote:
> >> +- clock-frequency: the frequency the timer is running
> >> +
> >> +Example:
> >> +	timer: timer@ff810000 {
> >> +		compatible = "rockchip,rk3288-timer";
> >> +		reg = <0xff810000 0x20>;
> >> +		interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> >> +		clock-frequency = <24000000>;
> > 
> > wouldn't it make sense to use the actual supplying clock?
> > 
> > For the timer you want to use it is just the non-gateable &xin24m, but the
> > timers in the other block (timer0-5) actually do have gateable clocks.
> > 
> > Similarly there is a pclk_timer supplying at least one of the two actual
> > blocks. I'll try to inquire how the blocks are actually supplied.
> 
> Ok, are you suggesting I should use another timer so we can gate it for
> power efficiency ?

As you want a timer that is running during suspend, I think the one you're 
using right now is the correct one ... the others (timer0-5) are in the cpu-
domain instead of the alive-domain

My intention was more to not have a clock-frequency property, but to simply 
use the correct cock frequency.

clocks = <&xin24m>, <&cru PCLK_TIMER>;
clock-names = "timer", "pclk";

[same binding as dw_apb_timer]


rockchip_timer.c:

tclk = of_clk_get_by_name(np, "timer");
clk_prepare_enable(tclk);
bc_timer.freq = clk_get_rate(tclk);

etc...

essentially like db_apb_timer_of.c does it :-)


We currently don't implement the gates Jack mentioned, but as they're open 
anyway we can just use xin24m directly for now.
And the pclk should probably be enabled too, as with 3.19-rc it gets disabled 
when unused.


Heiko
--
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