Add Freescale Vybrid MVF600 period interrupt timer support.
Signed-off-by: Jingchang Lu <[email protected]>
---
v3:
move the pit driver to drivers/clocksource.
drivers/clocksource/Kconfig | 5 +
drivers/clocksource/Makefile | 1 +
drivers/clocksource/mvf600_pit_timer.c | 224 +++++++++++++++++++++++++++++++++
3 files changed, 230 insertions(+)
create mode 100644 drivers/clocksource/mvf600_pit_timer.c
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index f151c6c..2808a76 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -85,3 +85,8 @@ config CLKSRC_SAMSUNG_PWM
Samsung S3C, S5P and Exynos SoCs, replacing an earlier driver
for all devicetree enabled platforms. This driver will be
needed only on systems that do not have the Exynos MCT available.
+
+config MVF600_PIT_TIMER
+ bool
+ help
+ Support for Period Interrupt Timer on Freescale Vybrid MVF600 SoC.
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8d979c7..f471ca3 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_ARCH_BCM) += bcm_kona_timer.o
obj-$(CONFIG_CADENCE_TTC_TIMER) += cadence_ttc_timer.o
obj-$(CONFIG_CLKSRC_EXYNOS_MCT) += exynos_mct.o
obj-$(CONFIG_CLKSRC_SAMSUNG_PWM) += samsung_pwm_timer.o
+obj-$(CONFIG_MVF600_PIT_TIMER) += mvf600_pit_timer.o
obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o
obj-$(CONFIG_CLKSRC_METAG_GENERIC) += metag_generic.o
diff --git a/drivers/clocksource/mvf600_pit_timer.c b/drivers/clocksource/mvf600_pit_timer.c
new file mode 100644
index 0000000..74f7963
--- /dev/null
+++ b/drivers/clocksource/mvf600_pit_timer.c
@@ -0,0 +1,224 @@
+/*
+ * Copyright 2012-2013 Freescale Semiconductor, 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; either version 2
+ * of the License, or (at your option) any later version.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/clockchips.h>
+#include <linux/clk.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <asm/sched_clock.h>
+
+/*
+ * 8 timers: pit0 - pit7,
+ * Each takes 0x10 Bytes register sapce
+ */
+#define PITMCR 0x00
+#define PIT0_OFFSET 0x100
+#define PITn_OFFSET(n) (PIT0_OFFSET + 0x10 * (n))
+#define PITLDVAL 0x00
+#define PITCVAL 0x04
+#define PITTCTRL 0x08
+#define PITTFLG 0x0c
+
+#define PITTCTRL_TEN (0x1 << 0)
+#define PITTCTRL_TIE (0x1 << 1)
+#define PITCTRL_CHN (0x1 << 2)
+
+#define PITTFLG_TIF 0x1
+
+static struct clock_event_device clockevent_pit;
+static enum clock_event_mode clockevent_mode = CLOCK_EVT_MODE_UNUSED;
+
+static void __iomem *clksrc_base;
+static void __iomem *clkevt_base;
+static void __iomem *sched_clock_reg;
+static unsigned long pit_cycle_per_jiffy;
+
+static inline void pit_timer_enable(void)
+{
+ __raw_writel(PITTCTRL_TEN | PITTCTRL_TIE, clkevt_base + PITTCTRL);
+}
+
+static inline void pit_timer_disable(void)
+{
+ __raw_writel(0, clkevt_base + PITTCTRL);
+}
+
+static inline void pit_irq_disable(void)
+{
+ unsigned long val;
+
+ val = __raw_readl(clkevt_base + PITTCTRL);
+ val &= ~PITTCTRL_TIE;
+ __raw_writel(val, clkevt_base + PITTCTRL);
+}
+
+static inline void pit_irq_enable(void)
+{
+ unsigned long val;
+
+ val = __raw_readl(clkevt_base + PITTCTRL);
+ val |= PITTCTRL_TIE;
+ __raw_writel(val, clkevt_base + PITTCTRL);
+}
+
+static void pit_irq_acknowledge(void)
+{
+ __raw_writel(PITTFLG_TIF, clkevt_base + PITTFLG);
+}
+
+static unsigned int pit_read_sched_clock(void)
+{
+ return __raw_readl(sched_clock_reg);
+}
+
+
+static int __init pit_clocksource_init(struct clk *pit_clk)
+{
+ unsigned int c = clk_get_rate(pit_clk);
+
+ sched_clock_reg = clksrc_base + PITCVAL;
+
+ setup_sched_clock(pit_read_sched_clock, 32, c);
+ return clocksource_mmio_init(clksrc_base + PITCVAL, "mvf600-pit", c,
+ 300, 32, clocksource_mmio_readl_down);
+}
+
+/* set clock event */
+static int pit_set_next_event(unsigned long delta,
+ struct clock_event_device *unused)
+{
+ pit_timer_disable();
+ __raw_writel(delta - 1, clkevt_base + PITLDVAL);
+ pit_irq_acknowledge();
+ pit_timer_enable();
+
+ return 0;
+}
+
+static void pit_set_mode(enum clock_event_mode mode,
+ struct clock_event_device *evt)
+{
+ unsigned long flags;
+
+ local_irq_save(flags);
+
+ pit_timer_disable();
+ pit_irq_acknowledge();
+
+ /* Remember timer mode */
+ clockevent_mode = mode;
+ local_irq_restore(flags);
+
+ switch (mode) {
+ case CLOCK_EVT_MODE_PERIODIC:
+
+ __raw_writel(pit_cycle_per_jiffy - 1, clkevt_base + PITLDVAL);
+ pit_timer_enable();
+
+ break;
+ case CLOCK_EVT_MODE_ONESHOT:
+
+ break;
+ case CLOCK_EVT_MODE_SHUTDOWN:
+ case CLOCK_EVT_MODE_UNUSED:
+ case CLOCK_EVT_MODE_RESUME:
+
+ break;
+ default:
+ WARN(1, "%s: unhandled event mode %d\n", __func__, mode);
+ break;
+ }
+}
+
+static irqreturn_t pit_timer_interrupt(int irq, void *dev_id)
+{
+ struct clock_event_device *evt = &clockevent_pit;
+
+ pit_irq_acknowledge();
+
+ if (clockevent_mode == CLOCK_EVT_MODE_ONESHOT)
+ pit_timer_disable();
+
+ evt->event_handler(evt);
+
+ return IRQ_HANDLED;
+}
+
+static struct irqaction pit_timer_irq = {
+ .name = "MVF600 pit timer",
+ .flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
+ .handler = pit_timer_interrupt,
+};
+
+static struct clock_event_device clockevent_pit = {
+ .name = "MVF600 pit timer",
+ .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+ .set_mode = pit_set_mode,
+ .set_next_event = pit_set_next_event,
+ .rating = 300,
+};
+
+static int __init pit_clockevent_init(struct clk *pit_clk)
+{
+ unsigned int c = clk_get_rate(pit_clk);
+
+ clockevent_pit.cpumask = cpumask_of(0);
+ clockevents_config_and_register(&clockevent_pit, c, 0x100, 0xffffff00);
+
+ return 0;
+}
+
+static void __init pit_timer_init(struct device_node *np)
+{
+ struct clk *pit_clk;
+ void __iomem *timer_base;
+ int irq;
+
+ if (!np) {
+ pr_err("Failed to find MVF600 pit DT node\n");
+ BUG();
+ }
+
+ timer_base = of_iomap(np, 0);
+ WARN_ON(!timer_base);
+
+ /* choose PIT2 as clocksource, PIT3 as clockevent dev */
+ clksrc_base = timer_base + PITn_OFFSET(2);
+ clkevt_base = timer_base + PITn_OFFSET(3);
+
+ irq = irq_of_parse_and_map(np, 0);
+
+ pit_clk = of_clk_get(np, 0);
+ if (IS_ERR(pit_clk)) {
+ pr_err("Vybrid MVF600 pit timer: unable to get clk\n");
+ return;
+ }
+
+ clk_prepare_enable(pit_clk);
+
+ pit_cycle_per_jiffy = clk_get_rate(pit_clk)/(HZ);
+
+ __raw_writel(0x0, timer_base + PITMCR);
+
+ __raw_writel(0, clkevt_base + PITTCTRL);
+ __raw_writel(0xffffffff, clkevt_base + PITLDVAL);
+
+ __raw_writel(0, clksrc_base + PITTCTRL);
+ __raw_writel(0xffffffff, clksrc_base + PITLDVAL);
+ __raw_writel(PITTCTRL_TEN, clksrc_base + PITTCTRL);
+
+ pit_clocksource_init(pit_clk);
+
+ setup_irq(irq, &pit_timer_irq);
+
+ pit_clockevent_init(pit_clk);
+}
+
+CLOCKSOURCE_OF_DECLARE(mvf600, "fsl,mvf600-pit", pit_timer_init);
--
1.8.0
On Thu, 16 May 2013, Jingchang Lu wrote:
> +/*
> + * 8 timers: pit0 - pit7,
> + * Each takes 0x10 Bytes register sapce
> + */
> +#define PITMCR 0x00
> +#define PIT0_OFFSET 0x100
> +#define PITn_OFFSET(n) (PIT0_OFFSET + 0x10 * (n))
> +#define PITLDVAL 0x00
> +#define PITCVAL 0x04
> +#define PITTCTRL 0x08
> +#define PITTFLG 0x0c
> +
> +#define PITTCTRL_TEN (0x1 << 0)
> +#define PITTCTRL_TIE (0x1 << 1)
> +#define PITCTRL_CHN (0x1 << 2)
> +
> +#define PITTFLG_TIF 0x1
> +
> +static struct clock_event_device clockevent_pit;
> +static enum clock_event_mode clockevent_mode = CLOCK_EVT_MODE_UNUSED;
What the heck is this? The clock event device itself tracks the
mode. So why do you need a separate status variable?
> +
> +static void __iomem *clksrc_base;
> +static void __iomem *clkevt_base;
> +static void __iomem *sched_clock_reg;
> +static unsigned long pit_cycle_per_jiffy;
> +
> +static inline void pit_timer_enable(void)
> +{
> + __raw_writel(PITTCTRL_TEN | PITTCTRL_TIE, clkevt_base + PITTCTRL);
> +}
> +
> +static inline void pit_timer_disable(void)
> +{
> + __raw_writel(0, clkevt_base + PITTCTRL);
> +}
> +
> +static inline void pit_irq_disable(void)
Unused function
> +{
> + unsigned long val;
> +
> + val = __raw_readl(clkevt_base + PITTCTRL);
> + val &= ~PITTCTRL_TIE;
> + __raw_writel(val, clkevt_base + PITTCTRL);
> +}
> +
> +static inline void pit_irq_enable(void)
Ditto
> +{
> + unsigned long val;
> +
> + val = __raw_readl(clkevt_base + PITTCTRL);
> + val |= PITTCTRL_TIE;
> + __raw_writel(val, clkevt_base + PITTCTRL);
> +}
> +
> +static void pit_irq_acknowledge(void)
inline
> +{
> + __raw_writel(PITTFLG_TIF, clkevt_base + PITTFLG);
> +}
> +
> +static unsigned int pit_read_sched_clock(void)
> +{
> + return __raw_readl(sched_clock_reg);
> +}
> +
> +
> +static int __init pit_clocksource_init(struct clk *pit_clk)
> +{
> + unsigned int c = clk_get_rate(pit_clk);
> +
> + sched_clock_reg = clksrc_base + PITCVAL;
> +
> + setup_sched_clock(pit_read_sched_clock, 32, c);
> + return clocksource_mmio_init(clksrc_base + PITCVAL, "mvf600-pit", c,
> + 300, 32, clocksource_mmio_readl_down);
> +}
> +
> +/* set clock event */
This is the most useless comment ever.
> +static int pit_set_next_event(unsigned long delta,
> + struct clock_event_device *unused)
> +{
> + pit_timer_disable();
> + __raw_writel(delta - 1, clkevt_base + PITLDVAL);
> + pit_irq_acknowledge();
> + pit_timer_enable();
It would be much more interesting to comment, why you need to
acknowlegde the timer here.
> + return 0;
> +}
> +
> +static void pit_set_mode(enum clock_event_mode mode,
> + struct clock_event_device *evt)
> +{
> + unsigned long flags;
> +
> + local_irq_save(flags);
All clockevent functions are called with interrupts disabled.
> + pit_timer_disable();
> + pit_irq_acknowledge();
> +
> + /* Remember timer mode */
> + clockevent_mode = mode;
Groan.
> + local_irq_restore(flags);
> +
> + switch (mode) {
> + case CLOCK_EVT_MODE_PERIODIC:
> +
> + __raw_writel(pit_cycle_per_jiffy - 1, clkevt_base + PITLDVAL);
> + pit_timer_enable();
> +
> + break;
> + case CLOCK_EVT_MODE_ONESHOT:
Whats so special that this needs a separate break?
> + break;
> + case CLOCK_EVT_MODE_SHUTDOWN:
> + case CLOCK_EVT_MODE_UNUSED:
> + case CLOCK_EVT_MODE_RESUME:
> +
> + break;
> + default:
> + WARN(1, "%s: unhandled event mode %d\n", __func__, mode);
What the heck? Either you have a default catching everything you do
not handle or you remove the default and let the compiler warn you
when the CLOCK_EVT_MODE enum got a new value added.
> + break;
> + }
> +}
> +
> +static irqreturn_t pit_timer_interrupt(int irq, void *dev_id)
> +{
> + struct clock_event_device *evt = &clockevent_pit;
> +
> + pit_irq_acknowledge();
> +
> + if (clockevent_mode == CLOCK_EVT_MODE_ONESHOT)
> + pit_timer_disable();
So in oneshot mode you do:
pit_irq_ack()
pit_timer_disable()
....
set_next_event()
pit_timer_disable()
write_new_value()
pit_irq_ack()
pit_timer_enable()
Not really the most efficient way in the interrupt fast path, right?
> + evt->event_handler(evt);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static struct irqaction pit_timer_irq = {
> + .name = "MVF600 pit timer",
> + .flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
Please look up what IRQF_DISABLED does and why you shouldn't use it.
> + .handler = pit_timer_interrupt,
> +};
> +
> +static struct clock_event_device clockevent_pit = {
> + .name = "MVF600 pit timer",
> + .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
> + .set_mode = pit_set_mode,
> + .set_next_event = pit_set_next_event,
> + .rating = 300,
> +};
> +
> +static int __init pit_clockevent_init(struct clk *pit_clk)
> +{
> + unsigned int c = clk_get_rate(pit_clk);
> +
> + clockevent_pit.cpumask = cpumask_of(0);
> + clockevents_config_and_register(&clockevent_pit, c, 0x100, 0xffffff00);
0x100 and 0xffffff00 ?? Random numbers pulled out of what?
> + return 0;
> +}
> +
> +static void __init pit_timer_init(struct device_node *np)
> +{
> + struct clk *pit_clk;
> + void __iomem *timer_base;
> + int irq;
> +
> + if (!np) {
So how gets that called without a valid node pointer?
> + pr_err("Failed to find MVF600 pit DT node\n");
> + BUG();
> + }
> +
> + timer_base = of_iomap(np, 0);
> + WARN_ON(!timer_base);
Great, timer_base is NULL and you just emit a warning and then
proceed? So instead of either bailing out or crashing the machine
right away you let it randomly die with the first access.
> +
> + /* choose PIT2 as clocksource, PIT3 as clockevent dev */
> + clksrc_base = timer_base + PITn_OFFSET(2);
> + clkevt_base = timer_base + PITn_OFFSET(3);
> +
> + irq = irq_of_parse_and_map(np, 0);
> +
> + pit_clk = of_clk_get(np, 0);
> + if (IS_ERR(pit_clk)) {
> + pr_err("Vybrid MVF600 pit timer: unable to get clk\n");
Can you please make your pr_*() format consistent?
> + pr_err("Failed to find MVF600 pit DT node\n");
> + pr_err("Vybrid MVF600 pit timer: unable to get clk\n");
> + return;
> + }
> +
> + clk_prepare_enable(pit_clk);
And while you're worried about the core code sending you random crap,
you assume that this call always succeeds.
> + pit_cycle_per_jiffy = clk_get_rate(pit_clk)/(HZ);
> +
> + __raw_writel(0x0, timer_base + PITMCR);
> +
> + __raw_writel(0, clkevt_base + PITTCTRL);
> + __raw_writel(0xffffffff, clkevt_base + PITLDVAL);
What's the point of this?
> + __raw_writel(0, clksrc_base + PITTCTRL);
> + __raw_writel(0xffffffff, clksrc_base + PITLDVAL);
And of this? Why isn't the setup done in the relevant init functions?
> + __raw_writel(PITTCTRL_TEN, clksrc_base + PITTCTRL);
> +
> + pit_clocksource_init(pit_clk);
> +
> + setup_irq(irq, &pit_timer_irq);
> +
> + pit_clockevent_init(pit_clk);
> +}
Thanks,
tglx
>-----Original Message-----
>From: Thomas Gleixner [mailto:[email protected]]
>Sent: Thursday, May 16, 2013 10:05 PM
>To: Lu Jingchang-B35083
>Cc: [email protected]; [email protected];
>[email protected]; [email protected]; [email protected]
>Subject: Re: [PATCH v3] clocksource: add MVF600 pit timer support
>
>> +static int pit_set_next_event(unsigned long delta,
>> + struct clock_event_device *unused) {
>> + pit_timer_disable();
>> + __raw_writel(delta - 1, clkevt_base + PITLDVAL);
>> + pit_irq_acknowledge();
>> + pit_timer_enable();
>
>It would be much more interesting to comment, why you need to acknowlegde
>the timer here.
[Lu Jingchang-B35083]
The pit is a period load and count timer, ack here is to make sure the int flag is clear,
I will check if the ack here is needed and remove the redundant code. Thanks!
>
>> +static irqreturn_t pit_timer_interrupt(int irq, void *dev_id) {
>> + struct clock_event_device *evt = &clockevent_pit;
>> +
>> + pit_irq_acknowledge();
>> +
>> + if (clockevent_mode == CLOCK_EVT_MODE_ONESHOT)
>> + pit_timer_disable();
>
>So in oneshot mode you do:
>
> pit_irq_ack()
> pit_timer_disable()
> ....
> set_next_event()
> pit_timer_disable()
> write_new_value()
> pit_irq_ack()
> pit_timer_enable()
>
>Not really the most efficient way in the interrupt fast path, right?
[Lu Jingchang-B35083]
The pit hardware doesn't support oneshot timer. So for oneshot mode event,
software need to disable and enable the timer each time.
The timers generate interrupt at periodic intervals, when enabled. The timers load the start
values as specified in their LDVAL registers, count down to 0, generate an INT and then reload the respective
start value again. Each time a timer reaches 0, it will set the interrupt flag.
I will remove the redundant irq_ack.
Thanks!
>
>> + evt->event_handler(evt);
>> +
>> + clockevents_config_and_register(&clockevent_pit, c, 0x100,
>> +0xffffff00);
>
>0x100 and 0xffffff00 ?? Random numbers pulled out of what?
[Lu Jingchang-B35083]
It seems too small delta is not reasonable, and the max delta value is not sensible.
So refer to some other platforms, I set it to 0x100-0xffffff00. I'm OK to set it to 0x2~0xffffffff.
Thanks!
>
>> +
>> + timer_base = of_iomap(np, 0);
>> + WARN_ON(!timer_base);
>
>Great, timer_base is NULL and you just emit a warning and then proceed? So
>instead of either bailing out or crashing the machine right away you let
>it randomly die with the first access.
[Lu Jingchang-B35083]
I will use BUG_ON() instead of WARN_ON(). Thanks!
>
>> +
>> + clk_prepare_enable(pit_clk);
>
>And while you're worried about the core code sending you random crap, you
>assume that this call always succeeds.
[Lu Jingchang-B35083]
I will check the return value for that call. Thanks!
>> + __raw_writel(0, clksrc_base + PITTCTRL);
>> + __raw_writel(0xffffffff, clksrc_base + PITLDVAL);
>
>And of this? Why isn't the setup done in the relevant init functions?
[Lu Jingchang-B35083]
I will move these to relevant init functions. Thanks!
On Thu, May 16, 2013 at 02:15:24PM +0800, Jingchang Lu wrote:
> Add Freescale Vybrid MVF600 period interrupt timer support.
>
> Signed-off-by: Jingchang Lu <[email protected]>
> ---
> v3:
> move the pit driver to drivers/clocksource.
>
> drivers/clocksource/Kconfig | 5 +
> drivers/clocksource/Makefile | 1 +
> drivers/clocksource/mvf600_pit_timer.c | 224 +++++++++++++++++++++++++++++++++
This is a driver for PIT, a timer IP block found on mvf family. Even
though we prefer to use particular SoC name to specify the compatibility
and version of the block, all the versions of the IP block should be
ideally supported by single PIT driver. This is just like we have
drivers spi-imx and i2c-imx support all SPI and I2C controllers found on
IMX SoCs.
That said, I suggest you use family name "mvf" to name the driver,
Kconfig symbol etc. and use "mvf600" only in compatible string.
> 3 files changed, 230 insertions(+)
> create mode 100644 drivers/clocksource/mvf600_pit_timer.c
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index f151c6c..2808a76 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -85,3 +85,8 @@ config CLKSRC_SAMSUNG_PWM
> Samsung S3C, S5P and Exynos SoCs, replacing an earlier driver
> for all devicetree enabled platforms. This driver will be
> needed only on systems that do not have the Exynos MCT available.
> +
> +config MVF600_PIT_TIMER
> + bool
> + help
> + Support for Period Interrupt Timer on Freescale Vybrid MVF600 SoC.
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 8d979c7..f471ca3 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_ARCH_BCM) += bcm_kona_timer.o
> obj-$(CONFIG_CADENCE_TTC_TIMER) += cadence_ttc_timer.o
> obj-$(CONFIG_CLKSRC_EXYNOS_MCT) += exynos_mct.o
> obj-$(CONFIG_CLKSRC_SAMSUNG_PWM) += samsung_pwm_timer.o
> +obj-$(CONFIG_MVF600_PIT_TIMER) += mvf600_pit_timer.o
>
> obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o
> obj-$(CONFIG_CLKSRC_METAG_GENERIC) += metag_generic.o
> diff --git a/drivers/clocksource/mvf600_pit_timer.c b/drivers/clocksource/mvf600_pit_timer.c
> new file mode 100644
> index 0000000..74f7963
> --- /dev/null
> +++ b/drivers/clocksource/mvf600_pit_timer.c
> @@ -0,0 +1,224 @@
> +/*
> + * Copyright 2012-2013 Freescale Semiconductor, 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; either version 2
> + * of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/clockchips.h>
> +#include <linux/clk.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <asm/sched_clock.h>
> +
> +/*
> + * 8 timers: pit0 - pit7,
> + * Each takes 0x10 Bytes register sapce
s/sapce/space
> + */
> +#define PITMCR 0x00
> +#define PIT0_OFFSET 0x100
> +#define PITn_OFFSET(n) (PIT0_OFFSET + 0x10 * (n))
> +#define PITLDVAL 0x00
> +#define PITCVAL 0x04
> +#define PITTCTRL 0x08
> +#define PITTFLG 0x0c
> +
> +#define PITTCTRL_TEN (0x1 << 0)
> +#define PITTCTRL_TIE (0x1 << 1)
> +#define PITCTRL_CHN (0x1 << 2)
Use space than tab between "#define" and macro name.
> +
> +#define PITTFLG_TIF 0x1
> +
> +static struct clock_event_device clockevent_pit;
> +static enum clock_event_mode clockevent_mode = CLOCK_EVT_MODE_UNUSED;
> +
> +static void __iomem *clksrc_base;
> +static void __iomem *clkevt_base;
> +static void __iomem *sched_clock_reg;
> +static unsigned long pit_cycle_per_jiffy;
> +
> +static inline void pit_timer_enable(void)
> +{
> + __raw_writel(PITTCTRL_TEN | PITTCTRL_TIE, clkevt_base + PITTCTRL);
> +}
> +
> +static inline void pit_timer_disable(void)
> +{
> + __raw_writel(0, clkevt_base + PITTCTRL);
> +}
> +
> +static inline void pit_irq_disable(void)
> +{
> + unsigned long val;
> +
> + val = __raw_readl(clkevt_base + PITTCTRL);
> + val &= ~PITTCTRL_TIE;
> + __raw_writel(val, clkevt_base + PITTCTRL);
> +}
> +
> +static inline void pit_irq_enable(void)
> +{
> + unsigned long val;
> +
> + val = __raw_readl(clkevt_base + PITTCTRL);
> + val |= PITTCTRL_TIE;
> + __raw_writel(val, clkevt_base + PITTCTRL);
> +}
> +
> +static void pit_irq_acknowledge(void)
> +{
> + __raw_writel(PITTFLG_TIF, clkevt_base + PITTFLG);
> +}
> +
> +static unsigned int pit_read_sched_clock(void)
> +{
> + return __raw_readl(sched_clock_reg);
> +}
> +
> +
> +static int __init pit_clocksource_init(struct clk *pit_clk)
> +{
> + unsigned int c = clk_get_rate(pit_clk);
> +
> + sched_clock_reg = clksrc_base + PITCVAL;
> +
> + setup_sched_clock(pit_read_sched_clock, 32, c);
> + return clocksource_mmio_init(clksrc_base + PITCVAL, "mvf600-pit", c,
> + 300, 32, clocksource_mmio_readl_down);
> +}
> +
> +/* set clock event */
> +static int pit_set_next_event(unsigned long delta,
> + struct clock_event_device *unused)
> +{
> + pit_timer_disable();
> + __raw_writel(delta - 1, clkevt_base + PITLDVAL);
> + pit_irq_acknowledge();
> + pit_timer_enable();
> +
> + return 0;
> +}
> +
> +static void pit_set_mode(enum clock_event_mode mode,
> + struct clock_event_device *evt)
> +{
> + unsigned long flags;
> +
> + local_irq_save(flags);
> +
> + pit_timer_disable();
> + pit_irq_acknowledge();
> +
> + /* Remember timer mode */
> + clockevent_mode = mode;
> + local_irq_restore(flags);
> +
> + switch (mode) {
> + case CLOCK_EVT_MODE_PERIODIC:
> +
Nit: remove this blank line.
> + __raw_writel(pit_cycle_per_jiffy - 1, clkevt_base + PITLDVAL);
> + pit_timer_enable();
> +
Ditto
> + break;
> + case CLOCK_EVT_MODE_ONESHOT:
> +
> + break;
> + case CLOCK_EVT_MODE_SHUTDOWN:
> + case CLOCK_EVT_MODE_UNUSED:
> + case CLOCK_EVT_MODE_RESUME:
> +
Ditto
> + break;
> + default:
> + WARN(1, "%s: unhandled event mode %d\n", __func__, mode);
> + break;
> + }
> +}
> +
> +static irqreturn_t pit_timer_interrupt(int irq, void *dev_id)
> +{
> + struct clock_event_device *evt = &clockevent_pit;
> +
> + pit_irq_acknowledge();
> +
> + if (clockevent_mode == CLOCK_EVT_MODE_ONESHOT)
> + pit_timer_disable();
> +
> + evt->event_handler(evt);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static struct irqaction pit_timer_irq = {
> + .name = "MVF600 pit timer",
> + .flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
> + .handler = pit_timer_interrupt,
> +};
> +
> +static struct clock_event_device clockevent_pit = {
> + .name = "MVF600 pit timer",
> + .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
> + .set_mode = pit_set_mode,
> + .set_next_event = pit_set_next_event,
> + .rating = 300,
> +};
> +
> +static int __init pit_clockevent_init(struct clk *pit_clk)
> +{
> + unsigned int c = clk_get_rate(pit_clk);
> +
> + clockevent_pit.cpumask = cpumask_of(0);
> + clockevents_config_and_register(&clockevent_pit, c, 0x100, 0xffffff00);
> +
> + return 0;
> +}
> +
> +static void __init pit_timer_init(struct device_node *np)
> +{
> + struct clk *pit_clk;
> + void __iomem *timer_base;
> + int irq;
> +
> + if (!np) {
> + pr_err("Failed to find MVF600 pit DT node\n");
> + BUG();
> + }
> +
> + timer_base = of_iomap(np, 0);
> + WARN_ON(!timer_base);
> +
> + /* choose PIT2 as clocksource, PIT3 as clockevent dev */
Reading the comment, I have a natural question - what PIT0 and PIT1 are
used for?
Shawn
> + clksrc_base = timer_base + PITn_OFFSET(2);
> + clkevt_base = timer_base + PITn_OFFSET(3);
> +
> + irq = irq_of_parse_and_map(np, 0);
> +
> + pit_clk = of_clk_get(np, 0);
> + if (IS_ERR(pit_clk)) {
> + pr_err("Vybrid MVF600 pit timer: unable to get clk\n");
> + return;
> + }
> +
> + clk_prepare_enable(pit_clk);
> +
> + pit_cycle_per_jiffy = clk_get_rate(pit_clk)/(HZ);
> +
> + __raw_writel(0x0, timer_base + PITMCR);
> +
> + __raw_writel(0, clkevt_base + PITTCTRL);
> + __raw_writel(0xffffffff, clkevt_base + PITLDVAL);
> +
> + __raw_writel(0, clksrc_base + PITTCTRL);
> + __raw_writel(0xffffffff, clksrc_base + PITLDVAL);
> + __raw_writel(PITTCTRL_TEN, clksrc_base + PITTCTRL);
> +
> + pit_clocksource_init(pit_clk);
> +
> + setup_irq(irq, &pit_timer_irq);
> +
> + pit_clockevent_init(pit_clk);
> +}
> +
> +CLOCKSOURCE_OF_DECLARE(mvf600, "fsl,mvf600-pit", pit_timer_init);
> --
> 1.8.0
>
>
>-----Original Message-----
>From: Shawn Guo [mailto:[email protected]]
>Sent: Monday, May 20, 2013 10:28 AM
>To: Lu Jingchang-B35083
>Cc: [email protected]; [email protected];
>[email protected]; [email protected]; [email protected]
>Subject: Re: [PATCH v3] clocksource: add MVF600 pit timer support
>
>On Thu, May 16, 2013 at 02:15:24PM +0800, Jingchang Lu wrote:
>> Add Freescale Vybrid MVF600 period interrupt timer support.
>>
>> Signed-off-by: Jingchang Lu <[email protected]>
>> ---
>> v3:
>> move the pit driver to drivers/clocksource.
>>
>> drivers/clocksource/Kconfig | 5 +
>> drivers/clocksource/Makefile | 1 +
>> drivers/clocksource/mvf600_pit_timer.c | 224
>> +++++++++++++++++++++++++++++++++
>
>This is a driver for PIT, a timer IP block found on mvf family. Even
>though we prefer to use particular SoC name to specify the compatibility
>and version of the block, all the versions of the IP block should be
>ideally supported by single PIT driver. This is just like we have drivers
>spi-imx and i2c-imx support all SPI and I2C controllers found on IMX SoCs.
>That said, I suggest you use family name "mvf" to name the driver, Kconfig
>symbol etc. and use "mvf600" only in compatible string.
>
[Lu Jingchang-B35083]
I am not sure MVF5xx and MVF7xx have the same PIT block, if you have information that it is the same on other Vybrid SoCs, it is ok to change the driver name to mvf. Thanks!
>> 3 files changed, 230 insertions(+)
>> +
>> + /* choose PIT2 as clocksource, PIT3 as clockevent dev */
>
>Reading the comment, I have a natural question - what PIT0 and PIT1 are
>used for?
>
[Lu Jingchang-B35083]
PIT0 and PIT1 can be chained to build a 64-bit timer, so PIT2 and PIT3 are selected as the clocksource and clockevent device, and leave PIT0 and PIT1 unused for anyone else who needs them. Thanks!
>
On Mon, May 20, 2013 at 03:08:54AM +0000, Lu Jingchang-B35083 wrote:
> [Lu Jingchang-B35083]
> I am not sure MVF5xx and MVF7xx have the same PIT block, if you have information that it is the same on other Vybrid SoCs, it is ok to change the driver name to mvf. Thanks!
Even the PIT block on other Vybrid SoCs have differences from the one
integrated on mvf600, we can handle them using device tree compatible
string. And that's why we need to encode SoC name in compatible.
> [Lu Jingchang-B35083]
> PIT0 and PIT1 can be chained to build a 64-bit timer, so PIT2 and PIT3 are selected as the clocksource and clockevent device, and leave PIT0 and PIT1 unused for anyone else who needs them. Thanks!
This could be an useful info to be in the comment.
Shawn
>-----Original Message-----
>From: Shawn Guo [mailto:[email protected]]
>Sent: Monday, May 20, 2013 11:21 AM
>To: Lu Jingchang-B35083
>Cc: [email protected]; [email protected];
>[email protected]; [email protected]; [email protected]
>Subject: Re: [PATCH v3] clocksource: add MVF600 pit timer support
>
>On Mon, May 20, 2013 at 03:08:54AM +0000, Lu Jingchang-B35083 wrote:
>> [Lu Jingchang-B35083]
>> I am not sure MVF5xx and MVF7xx have the same PIT block, if you have
>information that it is the same on other Vybrid SoCs, it is ok to change
>the driver name to mvf. Thanks!
>
>Even the PIT block on other Vybrid SoCs have differences from the one
>integrated on mvf600, we can handle them using device tree compatible
>string. And that's why we need to encode SoC name in compatible.
>
[Lu Jingchang-B35083]
Ok, I will rename the driver and the configure option, thanks!
>> [Lu Jingchang-B35083]
>> PIT0 and PIT1 can be chained to build a 64-bit timer, so PIT2 and PIT3
>are selected as the clocksource and clockevent device, and leave PIT0 and
>PIT1 unused for anyone else who needs them. Thanks!
>
>This could be an useful info to be in the comment.
>
[Lu Jingchang-B35083]
I will add a comment for this, thanks!