[PATCH v4] clocksource: add Vybrid Family pit timer support

[PATCH v4] clocksource: add Vybrid Family pit timer support

[Jingchang Lu]

Add Freescale Vybrid Family period interrupt timer support.

Signed-off-by: Jingchang Lu <b35083@freescale.com>
---
v4:
  use family name names driver and symbol instead of SoC name.
  remove redundant code.
  use BUG_ON instead of WARN_ON.
  add necessory comment information.

 drivers/clocksource/Kconfig         |   5 +
 drivers/clocksource/Makefile        |   1 +
 drivers/clocksource/mvf_pit_timer.c | 187 ++++++++++++++++++++++++++++++++++++
 3 files changed, 193 insertions(+)
 create mode 100644 drivers/clocksource/mvf_pit_timer.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index f151c6c..4f3ca11 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 MVF_PIT_TIMER
+	bool
+	help
+	  Support for Period Interrupt Timer on Freescale Vybrid Family SoCs.
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8d979c7..e28783c 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_MVF_PIT_TIMER)	+= mvf_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/mvf_pit_timer.c b/drivers/clocksource/mvf_pit_timer.c
new file mode 100644
index 0000000..34f4871
--- /dev/null
+++ b/drivers/clocksource/mvf_pit_timer.c
@@ -0,0 +1,187 @@
+/*
+ * 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>
+
+/*
+ * Each pit takes 0x10 Bytes register 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 PITMCR_MDIS	(0x1 << 1)
+
+#define PITTCTRL_TEN	(0x1 << 0)
+#define PITTCTRL_TIE	(0x1 << 1)
+#define PITCTRL_CHN	(0x1 << 2)
+
+#define PITTFLG_TIF	0x1
+
+static void __iomem *clksrc_base;
+static void __iomem *clkevt_base;
+static unsigned long 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_acknowledge(void)
+{
+	__raw_writel(PITTFLG_TIF, clkevt_base + PITTFLG);
+}
+
+static unsigned int pit_read_sched_clock(void)
+{
+	return __raw_readl(clksrc_base + PITCVAL);
+}
+
+static int __init pit_clocksource_init(struct clk *pit_clk)
+{
+	unsigned int c = clk_get_rate(pit_clk);
+
+	/* set the max load value and start the clock source counter */
+	__raw_writel(0, clksrc_base + PITTCTRL);
+	__raw_writel(~0UL, clksrc_base + PITLDVAL);
+	__raw_writel(PITTCTRL_TEN, clksrc_base + PITTCTRL);
+
+	setup_sched_clock(pit_read_sched_clock, 32, c);
+	return clocksource_mmio_init(clksrc_base + PITCVAL, "mvf-pit", c,
+			300, 32, clocksource_mmio_readl_down);
+}
+
+static int pit_set_next_event(unsigned long delta,
+				struct clock_event_device *unused)
+{
+	/*
+	 * set a new value to PITLDVAL register will not restart the timer,
+	 * to abort the current cycle and start a timer period with the new
+	 * value, the timer must be disabled and enabled again.
+	 * and the PITLAVAL should be set to delta minus one.
+	 */
+	pit_timer_disable();
+	__raw_writel(delta - 1, clkevt_base + PITLDVAL);
+	pit_timer_enable();
+
+	return 0;
+}
+
+static void pit_set_mode(enum clock_event_mode mode,
+				struct clock_event_device *evt)
+{
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+		pit_timer_disable();
+		__raw_writel(cycle_per_jiffy - 1, clkevt_base + PITLDVAL);
+		pit_timer_enable();
+		break;
+	default:
+		break;
+	}
+}
+
+static irqreturn_t pit_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+
+	pit_irq_acknowledge();
+
+	/*
+	 * pit hardware doesn't support oneshot, it will generate an interrupt
+	 * and reload the counter value from PITLDVAL when PITCVAL reach zero,
+	 * and start the counter again. So software need to disable the timer
+	 * to stop the counter loop in ONESHOT mode.
+	 */
+	if (likely(evt->mode == CLOCK_EVT_MODE_ONESHOT))
+		pit_timer_disable();
+
+	evt->event_handler(evt);
+
+	return IRQ_HANDLED;
+}
+
+static struct clock_event_device clockevent_pit = {
+	.name		= "MVF 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 struct irqaction pit_timer_irq = {
+	.name		= "MVF pit timer",
+	.flags		= IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
+	.handler	= pit_timer_interrupt,
+	.dev_id		= &clockevent_pit,
+};
+
+static int __init pit_clockevent_init(struct clk *pit_clk, int irq)
+{
+	unsigned int c = clk_get_rate(pit_clk);
+
+	__raw_writel(0, clkevt_base + PITTCTRL);
+	__raw_writel(PITTFLG_TIF, clkevt_base + PITTFLG);
+
+	clockevent_pit.cpumask = cpumask_of(0);
+	clockevents_config_and_register(&clockevent_pit, c, 2, 0xffffffff);
+
+	BUG_ON(setup_irq(irq, &pit_timer_irq));
+
+	return 0;
+}
+
+static void __init pit_timer_init(struct device_node *np)
+{
+	struct clk *pit_clk;
+	void __iomem *timer_base;
+	int irq;
+
+	timer_base = of_iomap(np, 0);
+	BUG_ON(!timer_base);
+
+	/*
+	 * PIT0 and PIT1 can be chained to build a 64-bit timer,
+	 * so choose PIT2 as clocksource, PIT3 as clockevent device,
+	 * and leave PIT0 and PIT1 unused for anyone else who needs them.
+	 */
+	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);
+	BUG_ON(IS_ERR(pit_clk));
+
+	BUG_ON(clk_prepare_enable(pit_clk));
+
+	cycle_per_jiffy = clk_get_rate(pit_clk)/(HZ);
+
+	/* enable the pit module */
+	__raw_writel(~PITMCR_MDIS, timer_base + PITMCR);
+
+	BUG_ON(pit_clocksource_init(pit_clk));
+
+	pit_clockevent_init(pit_clk, irq);
+}
+CLOCKSOURCE_OF_DECLARE(mvf600, "fsl,mvf600-pit", pit_timer_init);
-- 
1.8.0

[PATCH v4] clocksource: add Vybrid Family pit timer support

[Jingchang Lu]

Add Freescale Vybrid Family period interrupt timer support.

Signed-off-by: Jingchang Lu <b35083@freescale.com>
---
v4:
  use family name names driver and symbol instead of SoC name.
  remove redundant code.
  use BUG_ON instead of WARN_ON.
  add necessory comment information.

 drivers/clocksource/Kconfig         |   5 +
 drivers/clocksource/Makefile        |   1 +
 drivers/clocksource/mvf_pit_timer.c | 187 ++++++++++++++++++++++++++++++++++++
 3 files changed, 193 insertions(+)
 create mode 100644 drivers/clocksource/mvf_pit_timer.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index f151c6c..4f3ca11 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 MVF_PIT_TIMER
+	bool
+	help
+	  Support for Period Interrupt Timer on Freescale Vybrid Family SoCs.
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8d979c7..e28783c 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_MVF_PIT_TIMER)	+= mvf_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/mvf_pit_timer.c b/drivers/clocksource/mvf_pit_timer.c
new file mode 100644
index 0000000..34f4871
--- /dev/null
+++ b/drivers/clocksource/mvf_pit_timer.c
@@ -0,0 +1,187 @@
+/*
+ * 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>
+
+/*
+ * Each pit takes 0x10 Bytes register 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 PITMCR_MDIS	(0x1 << 1)
+
+#define PITTCTRL_TEN	(0x1 << 0)
+#define PITTCTRL_TIE	(0x1 << 1)
+#define PITCTRL_CHN	(0x1 << 2)
+
+#define PITTFLG_TIF	0x1
+
+static void __iomem *clksrc_base;
+static void __iomem *clkevt_base;
+static unsigned long 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_acknowledge(void)
+{
+	__raw_writel(PITTFLG_TIF, clkevt_base + PITTFLG);
+}
+
+static unsigned int pit_read_sched_clock(void)
+{
+	return __raw_readl(clksrc_base + PITCVAL);
+}
+
+static int __init pit_clocksource_init(struct clk *pit_clk)
+{
+	unsigned int c = clk_get_rate(pit_clk);
+
+	/* set the max load value and start the clock source counter */
+	__raw_writel(0, clksrc_base + PITTCTRL);
+	__raw_writel(~0UL, clksrc_base + PITLDVAL);
+	__raw_writel(PITTCTRL_TEN, clksrc_base + PITTCTRL);
+
+	setup_sched_clock(pit_read_sched_clock, 32, c);
+	return clocksource_mmio_init(clksrc_base + PITCVAL, "mvf-pit", c,
+			300, 32, clocksource_mmio_readl_down);
+}
+
+static int pit_set_next_event(unsigned long delta,
+				struct clock_event_device *unused)
+{
+	/*
+	 * set a new value to PITLDVAL register will not restart the timer,
+	 * to abort the current cycle and start a timer period with the new
+	 * value, the timer must be disabled and enabled again.
+	 * and the PITLAVAL should be set to delta minus one.
+	 */
+	pit_timer_disable();
+	__raw_writel(delta - 1, clkevt_base + PITLDVAL);
+	pit_timer_enable();
+
+	return 0;
+}
+
+static void pit_set_mode(enum clock_event_mode mode,
+				struct clock_event_device *evt)
+{
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+		pit_timer_disable();
+		__raw_writel(cycle_per_jiffy - 1, clkevt_base + PITLDVAL);
+		pit_timer_enable();
+		break;
+	default:
+		break;
+	}
+}
+
+static irqreturn_t pit_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+
+	pit_irq_acknowledge();
+
+	/*
+	 * pit hardware doesn't support oneshot, it will generate an interrupt
+	 * and reload the counter value from PITLDVAL when PITCVAL reach zero,
+	 * and start the counter again. So software need to disable the timer
+	 * to stop the counter loop in ONESHOT mode.
+	 */
+	if (likely(evt->mode == CLOCK_EVT_MODE_ONESHOT))
+		pit_timer_disable();
+
+	evt->event_handler(evt);
+
+	return IRQ_HANDLED;
+}
+
+static struct clock_event_device clockevent_pit = {
+	.name		= "MVF 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 struct irqaction pit_timer_irq = {
+	.name		= "MVF pit timer",
+	.flags		= IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
+	.handler	= pit_timer_interrupt,
+	.dev_id		= &clockevent_pit,
+};
+
+static int __init pit_clockevent_init(struct clk *pit_clk, int irq)
+{
+	unsigned int c = clk_get_rate(pit_clk);
+
+	__raw_writel(0, clkevt_base + PITTCTRL);
+	__raw_writel(PITTFLG_TIF, clkevt_base + PITTFLG);
+
+	clockevent_pit.cpumask = cpumask_of(0);
+	clockevents_config_and_register(&clockevent_pit, c, 2, 0xffffffff);
+
+	BUG_ON(setup_irq(irq, &pit_timer_irq));
+
+	return 0;
+}
+
+static void __init pit_timer_init(struct device_node *np)
+{
+	struct clk *pit_clk;
+	void __iomem *timer_base;
+	int irq;
+
+	timer_base = of_iomap(np, 0);
+	BUG_ON(!timer_base);
+
+	/*
+	 * PIT0 and PIT1 can be chained to build a 64-bit timer,
+	 * so choose PIT2 as clocksource, PIT3 as clockevent device,
+	 * and leave PIT0 and PIT1 unused for anyone else who needs them.
+	 */
+	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);
+	BUG_ON(IS_ERR(pit_clk));
+
+	BUG_ON(clk_prepare_enable(pit_clk));
+
+	cycle_per_jiffy = clk_get_rate(pit_clk)/(HZ);
+
+	/* enable the pit module */
+	__raw_writel(~PITMCR_MDIS, timer_base + PITMCR);
+
+	BUG_ON(pit_clocksource_init(pit_clk));
+
+	pit_clockevent_init(pit_clk, irq);
+}
+CLOCKSOURCE_OF_DECLARE(mvf600, "fsl,mvf600-pit", pit_timer_init);
-- 
1.8.0

Re: [PATCH v4] clocksource: add Vybrid Family pit timer support

[Daniel Lezcano]

On 05/21/2013 09:27 AM, Jingchang Lu wrote:
> Add Freescale Vybrid Family period interrupt timer support.
> 
> Signed-off-by: Jingchang Lu <b35083@freescale.com>
> ---
> v4:
>   use family name names driver and symbol instead of SoC name.
>   remove redundant code.
>   use BUG_ON instead of WARN_ON.
>   add necessory comment information.
> 
>  drivers/clocksource/Kconfig         |   5 +
>  drivers/clocksource/Makefile        |   1 +
>  drivers/clocksource/mvf_pit_timer.c | 187 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 193 insertions(+)
>  create mode 100644 drivers/clocksource/mvf_pit_timer.c

[ ... ]

> +
> +static int pit_set_next_event(unsigned long delta,
> +				struct clock_event_device *unused)
> +{
> +	/*
> +	 * set a new value to PITLDVAL register will not restart the timer,
> +	 * to abort the current cycle and start a timer period with the new
> +	 * value, the timer must be disabled and enabled again.
> +	 * and the PITLAVAL should be set to delta minus one.

Why delta "minus one" ?

> +	 */
> +	pit_timer_disable();
> +	__raw_writel(delta - 1, clkevt_base + PITLDVAL);
> +	pit_timer_enable();
> +
> +	return 0;
> +}
> +
> +static void pit_set_mode(enum clock_event_mode mode,
> +				struct clock_event_device *evt)
> +{
> +	switch (mode) {
> +	case CLOCK_EVT_MODE_PERIODIC:
> +		pit_timer_disable();
> +		__raw_writel(cycle_per_jiffy - 1, clkevt_base + PITLDVAL);
> +		pit_timer_enable();

You can call pit_set_next_event(cycle_per_jiffy, evt) instead of adding
redundant code, no ?

> +		break;
> +	default:
> +		break;
> +	}
> +}

[ ... ]

> +
> +static struct clock_event_device clockevent_pit = {
> +	.name		= "MVF 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 struct irqaction pit_timer_irq = {
> +	.name		= "MVF pit timer",
> +	.flags		= IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,

Did you take into account Thomas's comment ?

> +	.handler	= pit_timer_interrupt,
> +	.dev_id		= &clockevent_pit,
> +};
> +
> +static int __init pit_clockevent_init(struct clk *pit_clk, int irq)
> +{
> +	unsigned int c = clk_get_rate(pit_clk);
> +
> +	__raw_writel(0, clkevt_base + PITTCTRL);
> +	__raw_writel(PITTFLG_TIF, clkevt_base + PITTFLG);
> +
> +	clockevent_pit.cpumask = cpumask_of(0);

The irq initialization is missing.

	clockevent_pit.irq = irq;

> +	clockevents_config_and_register(&clockevent_pit, c, 2, 0xffffffff);

Is it possible to have a comment with the justification for these values ?

> +
> +	BUG_ON(setup_irq(irq, &pit_timer_irq));
> +	return 0;

Everything is ok if you can't setup your timer ?

> +}
> +
> +static void __init pit_timer_init(struct device_node *np)
> +{
> +	struct clk *pit_clk;
> +	void __iomem *timer_base;
> +	int irq;
> +
> +	timer_base = of_iomap(np, 0);
> +	BUG_ON(!timer_base);

You raise a bug and then you go ahead setting up the address with an
invalid value, leading to a random crash.

> +	/*
> +	 * PIT0 and PIT1 can be chained to build a 64-bit timer,
> +	 * so choose PIT2 as clocksource, PIT3 as clockevent device,
> +	 * and leave PIT0 and PIT1 unused for anyone else who needs them.
> +	 */
> +	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);
> +	BUG_ON(IS_ERR(pit_clk));
> +
> +	BUG_ON(clk_prepare_enable(pit_clk));

Same here.

> +	cycle_per_jiffy = clk_get_rate(pit_clk)/(HZ);
> +
> +	/* enable the pit module */
> +	__raw_writel(~PITMCR_MDIS, timer_base + PITMCR);
> +
> +	BUG_ON(pit_clocksource_init(pit_clk));
> +
> +	pit_clockevent_init(pit_clk, irq);

Please, remove these BUG_ON, this is inconsistent especially with a one
call init function. If pit_timer_init can't be initialized, just pr_err
+ BUG.

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

[PATCH v4] clocksource: add Vybrid Family pit timer support

[Daniel Lezcano]

On 05/21/2013 09:27 AM, Jingchang Lu wrote:
> Add Freescale Vybrid Family period interrupt timer support.
> 
> Signed-off-by: Jingchang Lu <b35083@freescale.com>
> ---
> v4:
>   use family name names driver and symbol instead of SoC name.
>   remove redundant code.
>   use BUG_ON instead of WARN_ON.
>   add necessory comment information.
> 
>  drivers/clocksource/Kconfig         |   5 +
>  drivers/clocksource/Makefile        |   1 +
>  drivers/clocksource/mvf_pit_timer.c | 187 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 193 insertions(+)
>  create mode 100644 drivers/clocksource/mvf_pit_timer.c

[ ... ]

> +
> +static int pit_set_next_event(unsigned long delta,
> +				struct clock_event_device *unused)
> +{
> +	/*
> +	 * set a new value to PITLDVAL register will not restart the timer,
> +	 * to abort the current cycle and start a timer period with the new
> +	 * value, the timer must be disabled and enabled again.
> +	 * and the PITLAVAL should be set to delta minus one.

Why delta "minus one" ?

> +	 */
> +	pit_timer_disable();
> +	__raw_writel(delta - 1, clkevt_base + PITLDVAL);
> +	pit_timer_enable();
> +
> +	return 0;
> +}
> +
> +static void pit_set_mode(enum clock_event_mode mode,
> +				struct clock_event_device *evt)
> +{
> +	switch (mode) {
> +	case CLOCK_EVT_MODE_PERIODIC:
> +		pit_timer_disable();
> +		__raw_writel(cycle_per_jiffy - 1, clkevt_base + PITLDVAL);
> +		pit_timer_enable();

You can call pit_set_next_event(cycle_per_jiffy, evt) instead of adding
redundant code, no ?

> +		break;
> +	default:
> +		break;
> +	}
> +}

[ ... ]

> +
> +static struct clock_event_device clockevent_pit = {
> +	.name		= "MVF 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 struct irqaction pit_timer_irq = {
> +	.name		= "MVF pit timer",
> +	.flags		= IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,

Did you take into account Thomas's comment ?

> +	.handler	= pit_timer_interrupt,
> +	.dev_id		= &clockevent_pit,
> +};
> +
> +static int __init pit_clockevent_init(struct clk *pit_clk, int irq)
> +{
> +	unsigned int c = clk_get_rate(pit_clk);
> +
> +	__raw_writel(0, clkevt_base + PITTCTRL);
> +	__raw_writel(PITTFLG_TIF, clkevt_base + PITTFLG);
> +
> +	clockevent_pit.cpumask = cpumask_of(0);

The irq initialization is missing.

	clockevent_pit.irq = irq;

> +	clockevents_config_and_register(&clockevent_pit, c, 2, 0xffffffff);

Is it possible to have a comment with the justification for these values ?

> +
> +	BUG_ON(setup_irq(irq, &pit_timer_irq));
> +	return 0;

Everything is ok if you can't setup your timer ?

> +}
> +
> +static void __init pit_timer_init(struct device_node *np)
> +{
> +	struct clk *pit_clk;
> +	void __iomem *timer_base;
> +	int irq;
> +
> +	timer_base = of_iomap(np, 0);
> +	BUG_ON(!timer_base);

You raise a bug and then you go ahead setting up the address with an
invalid value, leading to a random crash.

> +	/*
> +	 * PIT0 and PIT1 can be chained to build a 64-bit timer,
> +	 * so choose PIT2 as clocksource, PIT3 as clockevent device,
> +	 * and leave PIT0 and PIT1 unused for anyone else who needs them.
> +	 */
> +	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);
> +	BUG_ON(IS_ERR(pit_clk));
> +
> +	BUG_ON(clk_prepare_enable(pit_clk));

Same here.

> +	cycle_per_jiffy = clk_get_rate(pit_clk)/(HZ);
> +
> +	/* enable the pit module */
> +	__raw_writel(~PITMCR_MDIS, timer_base + PITMCR);
> +
> +	BUG_ON(pit_clocksource_init(pit_clk));
> +
> +	pit_clockevent_init(pit_clk, irq);

Please, remove these BUG_ON, this is inconsistent especially with a one
call init function. If pit_timer_init can't be initialized, just pr_err
+ BUG.

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

RE: [PATCH v4] clocksource: add Vybrid Family pit timer support

[Lu Jingchang-B35083]

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



>-----Original Message-----
>From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
>Sent: Tuesday, May 21, 2013 6:14 PM
>To: Lu Jingchang-B35083
>Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>john.stultz@linaro.org; tglx@linutronix.de; shawn.guo@linaro.org;
>s.hauer@pengutronix.de
>Subject: Re: [PATCH v4] clocksource: add Vybrid Family pit timer support
>
>On 05/21/2013 09:27 AM, Jingchang Lu wrote:
>> Add Freescale Vybrid Family period interrupt timer support.
>>
>> Signed-off-by: Jingchang Lu <b35083@freescale.com>
>> ---
>> v4:
>>   use family name names driver and symbol instead of SoC name.
>>   remove redundant code.
>>   use BUG_ON instead of WARN_ON.
>>   add necessory comment information.
>>
>>  drivers/clocksource/Kconfig         |   5 +
>>  drivers/clocksource/Makefile        |   1 +
>>  drivers/clocksource/mvf_pit_timer.c | 187
>> ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 193 insertions(+)
>>  create mode 100644 drivers/clocksource/mvf_pit_timer.c
>
>[ ... ]
>
>> +
>> +static int pit_set_next_event(unsigned long delta,
>> +				struct clock_event_device *unused) {
>> +	/*
>> +	 * set a new value to PITLDVAL register will not restart the timer,
>> +	 * to abort the current cycle and start a timer period with the new
>> +	 * value, the timer must be disabled and enabled again.
>> +	 * and the PITLAVAL should be set to delta minus one.
>
>Why delta "minus one" ?
[Lu Jingchang-B35083] 
This is required by the PIT hardware, it is a down counter, the PITLAVAL register should be set to (delta-1), it will raise an interrupt when it counts down to 0. Thanks!
>
>> +	 */
>> +	pit_timer_disable();
>> +	__raw_writel(delta - 1, clkevt_base + PITLDVAL);
>> +	pit_timer_enable();
>> +
>> +	return 0;
>> +}
>> +
>> +static void pit_set_mode(enum clock_event_mode mode,
>> +				struct clock_event_device *evt)
>> +{
>> +	switch (mode) {
>> +	case CLOCK_EVT_MODE_PERIODIC:
>> +		pit_timer_disable();
>> +		__raw_writel(cycle_per_jiffy - 1, clkevt_base + PITLDVAL);
>> +		pit_timer_enable();
>
>You can call pit_set_next_event(cycle_per_jiffy, evt) instead of adding
>redundant code, no ?
[Lu Jingchang-B35083] 
 Yes, I will do that. Thanks!
>
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
>
>[ ... ]
>
>> +
>> +static struct clock_event_device clockevent_pit = {
>> +	.name		= "MVF 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 struct irqaction pit_timer_irq = {
>> +	.name		= "MVF pit timer",
>> +	.flags		= IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
>
>Did you take into account Thomas's comment ?
[Lu Jingchang-B35083] 
Sorry, I misunderstand Thomas's meaning, I will remove IRQF_DISABLED flag. Thanks! 
>
>> +	.handler	= pit_timer_interrupt,
>> +	.dev_id		= &clockevent_pit,
>> +};
>> +
>> +static int __init pit_clockevent_init(struct clk *pit_clk, int irq) {
>> +	unsigned int c = clk_get_rate(pit_clk);
>> +
>> +	__raw_writel(0, clkevt_base + PITTCTRL);
>> +	__raw_writel(PITTFLG_TIF, clkevt_base + PITTFLG);
>> +
>> +	clockevent_pit.cpumask = cpumask_of(0);
>
>The irq initialization is missing.
>
>	clockevent_pit.irq = irq;
>
[Lu Jingchang-B35083] 
Yes, I will add this. Thanks!

>> +	clockevents_config_and_register(&clockevent_pit, c, 2, 0xffffffff);
>
>Is it possible to have a comment with the justification for these values ?
[Lu Jingchang-B35083] 
Yes, I will add a comment for these values. Thanks!
>
>> +
>> +	BUG_ON(setup_irq(irq, &pit_timer_irq));
>> +	return 0;
>
>Everything is ok if you can't setup your timer ?
>
>> +}
>> +
>> +static void __init pit_timer_init(struct device_node *np) {
>> +	struct clk *pit_clk;
>> +	void __iomem *timer_base;
>> +	int irq;
>> +
>> +	timer_base = of_iomap(np, 0);
>> +	BUG_ON(!timer_base);
>
>You raise a bug and then you go ahead setting up the address with an
>invalid value, leading to a random crash.
[Lu Jingchang-B35083] 
The BUG_ON() will call BUG() if condition is true. It will print the stack trace and the current process will die, it won't go ahead, won't it? Thanks! 
>
>> +	/*
>> +	 * PIT0 and PIT1 can be chained to build a 64-bit timer,
>> +	 * so choose PIT2 as clocksource, PIT3 as clockevent device,
>> +	 * and leave PIT0 and PIT1 unused for anyone else who needs them.
>> +	 */
>> +	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);
>> +	BUG_ON(IS_ERR(pit_clk));
>> +
>> +	BUG_ON(clk_prepare_enable(pit_clk));
>
>Same here.
>
>> +	cycle_per_jiffy = clk_get_rate(pit_clk)/(HZ);
>> +
>> +	/* enable the pit module */
>> +	__raw_writel(~PITMCR_MDIS, timer_base + PITMCR);
>> +
>> +	BUG_ON(pit_clocksource_init(pit_clk));
>> +
>> +	pit_clockevent_init(pit_clk, irq);
>
>Please, remove these BUG_ON, this is inconsistent especially with a one
>call init function. If pit_timer_init can't be initialized, just pr_err
>+ BUG.
[Lu Jingchang-B35083] 
Do you mean that I should not use the BUG_ON or I need print an error information by pr_err before BUG. Thanks!
>
>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
>

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH v4] clocksource: add Vybrid Family pit timer support

[Lu Jingchang-B35083]

>-----Original Message-----
>From: Daniel Lezcano [mailto:daniel.lezcano at linaro.org]
>Sent: Tuesday, May 21, 2013 6:14 PM
>To: Lu Jingchang-B35083
>Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
>john.stultz at linaro.org; tglx at linutronix.de; shawn.guo at linaro.org;
>s.hauer at pengutronix.de
>Subject: Re: [PATCH v4] clocksource: add Vybrid Family pit timer support
>
>On 05/21/2013 09:27 AM, Jingchang Lu wrote:
>> Add Freescale Vybrid Family period interrupt timer support.
>>
>> Signed-off-by: Jingchang Lu <b35083@freescale.com>
>> ---
>> v4:
>>   use family name names driver and symbol instead of SoC name.
>>   remove redundant code.
>>   use BUG_ON instead of WARN_ON.
>>   add necessory comment information.
>>
>>  drivers/clocksource/Kconfig         |   5 +
>>  drivers/clocksource/Makefile        |   1 +
>>  drivers/clocksource/mvf_pit_timer.c | 187
>> ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 193 insertions(+)
>>  create mode 100644 drivers/clocksource/mvf_pit_timer.c
>
>[ ... ]
>
>> +
>> +static int pit_set_next_event(unsigned long delta,
>> +				struct clock_event_device *unused) {
>> +	/*
>> +	 * set a new value to PITLDVAL register will not restart the timer,
>> +	 * to abort the current cycle and start a timer period with the new
>> +	 * value, the timer must be disabled and enabled again.
>> +	 * and the PITLAVAL should be set to delta minus one.
>
>Why delta "minus one" ?
[Lu Jingchang-B35083] 
This is required by the PIT hardware, it is a down counter, the PITLAVAL register should be set to (delta-1), it will raise an interrupt when it counts down to 0. Thanks!
>
>> +	 */
>> +	pit_timer_disable();
>> +	__raw_writel(delta - 1, clkevt_base + PITLDVAL);
>> +	pit_timer_enable();
>> +
>> +	return 0;
>> +}
>> +
>> +static void pit_set_mode(enum clock_event_mode mode,
>> +				struct clock_event_device *evt)
>> +{
>> +	switch (mode) {
>> +	case CLOCK_EVT_MODE_PERIODIC:
>> +		pit_timer_disable();
>> +		__raw_writel(cycle_per_jiffy - 1, clkevt_base + PITLDVAL);
>> +		pit_timer_enable();
>
>You can call pit_set_next_event(cycle_per_jiffy, evt) instead of adding
>redundant code, no ?
[Lu Jingchang-B35083] 
 Yes, I will do that. Thanks!
>
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
>
>[ ... ]
>
>> +
>> +static struct clock_event_device clockevent_pit = {
>> +	.name		= "MVF 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 struct irqaction pit_timer_irq = {
>> +	.name		= "MVF pit timer",
>> +	.flags		= IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
>
>Did you take into account Thomas's comment ?
[Lu Jingchang-B35083] 
Sorry, I misunderstand Thomas's meaning, I will remove IRQF_DISABLED flag. Thanks! 
>
>> +	.handler	= pit_timer_interrupt,
>> +	.dev_id		= &clockevent_pit,
>> +};
>> +
>> +static int __init pit_clockevent_init(struct clk *pit_clk, int irq) {
>> +	unsigned int c = clk_get_rate(pit_clk);
>> +
>> +	__raw_writel(0, clkevt_base + PITTCTRL);
>> +	__raw_writel(PITTFLG_TIF, clkevt_base + PITTFLG);
>> +
>> +	clockevent_pit.cpumask = cpumask_of(0);
>
>The irq initialization is missing.
>
>	clockevent_pit.irq = irq;
>
[Lu Jingchang-B35083] 
Yes, I will add this. Thanks!

>> +	clockevents_config_and_register(&clockevent_pit, c, 2, 0xffffffff);
>
>Is it possible to have a comment with the justification for these values ?
[Lu Jingchang-B35083] 
Yes, I will add a comment for these values. Thanks!
>
>> +
>> +	BUG_ON(setup_irq(irq, &pit_timer_irq));
>> +	return 0;
>
>Everything is ok if you can't setup your timer ?
>
>> +}
>> +
>> +static void __init pit_timer_init(struct device_node *np) {
>> +	struct clk *pit_clk;
>> +	void __iomem *timer_base;
>> +	int irq;
>> +
>> +	timer_base = of_iomap(np, 0);
>> +	BUG_ON(!timer_base);
>
>You raise a bug and then you go ahead setting up the address with an
>invalid value, leading to a random crash.
[Lu Jingchang-B35083] 
The BUG_ON() will call BUG() if condition is true. It will print the stack trace and the current process will die, it won't go ahead, won't it? Thanks! 
>
>> +	/*
>> +	 * PIT0 and PIT1 can be chained to build a 64-bit timer,
>> +	 * so choose PIT2 as clocksource, PIT3 as clockevent device,
>> +	 * and leave PIT0 and PIT1 unused for anyone else who needs them.
>> +	 */
>> +	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);
>> +	BUG_ON(IS_ERR(pit_clk));
>> +
>> +	BUG_ON(clk_prepare_enable(pit_clk));
>
>Same here.
>
>> +	cycle_per_jiffy = clk_get_rate(pit_clk)/(HZ);
>> +
>> +	/* enable the pit module */
>> +	__raw_writel(~PITMCR_MDIS, timer_base + PITMCR);
>> +
>> +	BUG_ON(pit_clocksource_init(pit_clk));
>> +
>> +	pit_clockevent_init(pit_clk, irq);
>
>Please, remove these BUG_ON, this is inconsistent especially with a one
>call init function. If pit_timer_init can't be initialized, just pr_err
>+ BUG.
[Lu Jingchang-B35083] 
Do you mean that I should not use the BUG_ON or I need print an error information by pr_err before BUG. Thanks!
>
>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
>

Re: [PATCH v4] clocksource: add Vybrid Family pit timer support

[Daniel Lezcano]

On 05/22/2013 11:47 AM, Lu Jingchang-B35083 wrote:
> 

[ ... ]

>>
>>> +}
>>> +
>>> +static void __init pit_timer_init(struct device_node *np) {
>>> +	struct clk *pit_clk;
>>> +	void __iomem *timer_base;
>>> +	int irq;
>>> +
>>> +	timer_base = of_iomap(np, 0);
>>> +	BUG_ON(!timer_base);
>>
>> You raise a bug and then you go ahead setting up the address with an
>> invalid value, leading to a random crash.
> [Lu Jingchang-B35083] 
> The BUG_ON() will call BUG() if condition is true. It will print the stack trace and the current process will die, it won't go ahead, won't it? Thanks! 

Pff, right. I just puzzled myself. Never mind.


>>> +	/*
>>> +	 * PIT0 and PIT1 can be chained to build a 64-bit timer,
>>> +	 * so choose PIT2 as clocksource, PIT3 as clockevent device,
>>> +	 * and leave PIT0 and PIT1 unused for anyone else who needs them.
>>> +	 */
>>> +	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);
>>> +	BUG_ON(IS_ERR(pit_clk));
>>> +
>>> +	BUG_ON(clk_prepare_enable(pit_clk));
>>
>> Same here.
>>
>>> +	cycle_per_jiffy = clk_get_rate(pit_clk)/(HZ);
>>> +
>>> +	/* enable the pit module */
>>> +	__raw_writel(~PITMCR_MDIS, timer_base + PITMCR);
>>> +
>>> +	BUG_ON(pit_clocksource_init(pit_clk));
>>> +
>>> +	pit_clockevent_init(pit_clk, irq);
>>
>> Please, remove these BUG_ON, this is inconsistent especially with a one
>> call init function. If pit_timer_init can't be initialized, just pr_err
>> + BUG.
> [Lu Jingchang-B35083] 
> Do you mean that I should not use the BUG_ON or I need print an error information by pr_err before BUG. Thanks!

Actually, my initial comment was wrong, so you can ignore it.

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

[PATCH v4] clocksource: add Vybrid Family pit timer support

[Daniel Lezcano]

On 05/22/2013 11:47 AM, Lu Jingchang-B35083 wrote:
> 

[ ... ]

>>
>>> +}
>>> +
>>> +static void __init pit_timer_init(struct device_node *np) {
>>> +	struct clk *pit_clk;
>>> +	void __iomem *timer_base;
>>> +	int irq;
>>> +
>>> +	timer_base = of_iomap(np, 0);
>>> +	BUG_ON(!timer_base);
>>
>> You raise a bug and then you go ahead setting up the address with an
>> invalid value, leading to a random crash.
> [Lu Jingchang-B35083] 
> The BUG_ON() will call BUG() if condition is true. It will print the stack trace and the current process will die, it won't go ahead, won't it? Thanks! 

Pff, right. I just puzzled myself. Never mind.


>>> +	/*
>>> +	 * PIT0 and PIT1 can be chained to build a 64-bit timer,
>>> +	 * so choose PIT2 as clocksource, PIT3 as clockevent device,
>>> +	 * and leave PIT0 and PIT1 unused for anyone else who needs them.
>>> +	 */
>>> +	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);
>>> +	BUG_ON(IS_ERR(pit_clk));
>>> +
>>> +	BUG_ON(clk_prepare_enable(pit_clk));
>>
>> Same here.
>>
>>> +	cycle_per_jiffy = clk_get_rate(pit_clk)/(HZ);
>>> +
>>> +	/* enable the pit module */
>>> +	__raw_writel(~PITMCR_MDIS, timer_base + PITMCR);
>>> +
>>> +	BUG_ON(pit_clocksource_init(pit_clk));
>>> +
>>> +	pit_clockevent_init(pit_clk, irq);
>>
>> Please, remove these BUG_ON, this is inconsistent especially with a one
>> call init function. If pit_timer_init can't be initialized, just pr_err
>> + BUG.
> [Lu Jingchang-B35083] 
> Do you mean that I should not use the BUG_ON or I need print an error information by pr_err before BUG. Thanks!

Actually, my initial comment was wrong, so you can ignore it.

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