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

From: Heiko Stübner
Date: Mon Jan 12 2015 - 18:20:46 EST

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@xxxxxxxxxxxxxxxxxxx) 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@xxxxxxxxxx>
> ---
> .../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@xxxxxxxxxx>
> + *
> + * 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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/