[PATCH] clocksource: SuperH TMU Timer driver

[PATCH] clocksource: SuperH TMU Timer driver

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

This patch adds a TMU driver for the SuperH architecture.

The TMU driver is a platform driver with early platform
support to allow using a TMU channel as clockevent or
clocksource during system bootup or later.

Clocksource or clockevent can be selected.
Both periodic and oneshot clockevents are supported.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 Tested on a Migo-R board with sh7722.
 Applies on top of the MTU2 patches.
 Clocksource operation requires the following patch:
 "clocksource: setup mult_orig in clocksource_enable()"

 arch/sh/Kconfig              |   12 +
 drivers/clocksource/Makefile |    1 
 drivers/clocksource/sh_tmu.c |  461 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/sh_tmu.h       |   13 +
 4 files changed, 487 insertions(+)

--- 0006/arch/sh/Kconfig
+++ work/arch/sh/Kconfig	2009-05-01 13:18:03.000000000 +0900
@@ -116,6 +116,9 @@ config SYS_SUPPORTS_CMT
 config SYS_SUPPORTS_MTU2
 	bool
 
+config SYS_SUPPORTS_TMU
+	bool
+
 config STACKTRACE_SUPPORT
 	def_bool y
 
@@ -470,6 +473,15 @@ config SH_TMU
 	help
 	  This enables the use of the TMU as the system timer.
 
+config SH_TIMER_TMU
+	bool "TMU timer driver"
+	depends on !SH_TMU && SYS_SUPPORTS_TMU
+	default y
+	select GENERIC_TIME
+	select GENERIC_CLOCKEVENTS
+	help
+	  This enables the build of the TMU timer driver.
+
 config SH_TIMER_CMT
 	bool "CMT timer driver"
 	depends on SYS_SUPPORTS_CMT
--- 0004/drivers/clocksource/Makefile
+++ work/drivers/clocksource/Makefile	2009-05-01 13:17:38.000000000 +0900
@@ -4,3 +4,4 @@ obj-$(CONFIG_X86_PM_TIMER)	+= acpi_pm.o
 obj-$(CONFIG_SCx200HR_TIMER)	+= scx200_hrt.o
 obj-$(CONFIG_SH_TIMER_CMT)	+= sh_cmt.o
 obj-$(CONFIG_SH_TIMER_MTU2)	+= sh_mtu2.o
+obj-$(CONFIG_SH_TIMER_TMU)	+= sh_tmu.o
--- /dev/null
+++ work/drivers/clocksource/sh_tmu.c	2009-05-01 13:20:00.000000000 +0900
@@ -0,0 +1,461 @@
+/*
+ * SuperH Timer Support - TMU
+ *
+ *  Copyright (C) 2009 Magnus Damm
+ *
+ * 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
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/interrupt.h>
+#include <linux/ioport.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/irq.h>
+#include <linux/err.h>
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
+#include <linux/sh_tmu.h>
+
+struct sh_tmu_priv {
+	void __iomem *mapbase;
+	struct clk *clk;
+	struct irqaction irqaction;
+	struct platform_device *pdev;
+	unsigned long rate;
+	unsigned long periodic;
+	struct clock_event_device ced;
+	struct clocksource cs;
+};
+
+static DEFINE_SPINLOCK(sh_tmu_lock);
+
+#define TSTR -1 /* shared register */
+#define TCOR  0 /* channel register */
+#define TCNT 1 /* channel register */
+#define TCR 2 /* channel register */
+
+static inline unsigned long sh_tmu_read(struct sh_tmu_priv *p, int reg_nr)
+{
+	struct sh_tmu_config *cfg = p->pdev->dev.platform_data;
+	void __iomem *base = p->mapbase;
+	unsigned long offs;
+
+	if (reg_nr = TSTR)
+		return ioread8(base - cfg->channel_offset);
+
+	offs = reg_nr << 2;
+
+	if (reg_nr = TCR)
+		return ioread16(base + offs);
+	else
+		return ioread32(base + offs);
+}
+
+static inline void sh_tmu_write(struct sh_tmu_priv *p, int reg_nr,
+				unsigned long value)
+{
+	struct sh_tmu_config *cfg = p->pdev->dev.platform_data;
+	void __iomem *base = p->mapbase;
+	unsigned long offs;
+
+	if (reg_nr = TSTR) {
+		iowrite8(value, base - cfg->channel_offset);
+		return;
+	}
+
+	offs = reg_nr << 2;
+
+	if (reg_nr = TCR)
+		iowrite16(value, base + offs);
+	else
+		iowrite32(value, base + offs);
+}
+
+static void sh_tmu_start_stop_ch(struct sh_tmu_priv *p, int start)
+{
+	struct sh_tmu_config *cfg = p->pdev->dev.platform_data;
+	unsigned long flags, value;
+
+	/* start stop register shared by multiple timer channels */
+	spin_lock_irqsave(&sh_tmu_lock, flags);
+	value = sh_tmu_read(p, TSTR);
+
+	if (start)
+		value |= 1 << cfg->timer_bit;
+	else
+		value &= ~(1 << cfg->timer_bit);
+
+	sh_tmu_write(p, TSTR, value);
+	spin_unlock_irqrestore(&sh_tmu_lock, flags);
+}
+
+static int sh_tmu_enable(struct sh_tmu_priv *p)
+{
+	struct sh_tmu_config *cfg = p->pdev->dev.platform_data;
+	int ret;
+
+	/* enable clock */
+	ret = clk_enable(p->clk);
+	if (ret) {
+		pr_err("sh_tmu: cannot enable clock \"%s\"\n", cfg->clk);
+		return ret;
+	}
+
+	/* make sure channel is disabled */
+	sh_tmu_start_stop_ch(p, 0);
+
+	/* maximum timeout */
+	sh_tmu_write(p, TCOR, 0xffffffff);
+	sh_tmu_write(p, TCNT, 0xffffffff);
+
+	/* configure channel to parent clock / 4, irq off */
+	p->rate = clk_get_rate(p->clk) / 4;
+	sh_tmu_write(p, TCR, 0x0000);
+
+	/* enable channel */
+	sh_tmu_start_stop_ch(p, 1);
+
+	return 0;
+}
+
+static void sh_tmu_disable(struct sh_tmu_priv *p)
+{
+	/* disable channel */
+	sh_tmu_start_stop_ch(p, 0);
+
+	/* stop clock */
+	clk_disable(p->clk);
+}
+
+static void sh_tmu_set_next(struct sh_tmu_priv *p, unsigned long delta,
+			    int periodic)
+{
+	/* stop timer */
+	sh_tmu_start_stop_ch(p, 0);
+
+	/* acknowledge interrupt */
+	sh_tmu_read(p, TCR);
+
+	/* enable interrupt */
+	sh_tmu_write(p, TCR, 0x0020);
+
+	/* reload delta value in case of periodic timer */
+	if (periodic)
+		sh_tmu_write(p, TCOR, delta);
+	else
+		sh_tmu_write(p, TCOR, 0);
+
+	sh_tmu_write(p, TCNT, delta);
+
+	/* start timer */
+	sh_tmu_start_stop_ch(p, 1);
+}
+
+static irqreturn_t sh_tmu_interrupt(int irq, void *dev_id)
+{
+	struct sh_tmu_priv *p = dev_id;
+
+	/* disable or acknowledge interrupt */
+	if (p->ced.mode = CLOCK_EVT_MODE_ONESHOT)
+		sh_tmu_write(p, TCR, 0x0000);
+	else
+		sh_tmu_write(p, TCR, 0x0020);
+
+	/* notify clockevent layer */
+	p->ced.event_handler(&p->ced);
+	return IRQ_HANDLED;
+}
+
+static struct sh_tmu_priv *cs_to_sh_tmu(struct clocksource *cs)
+{
+	return container_of(cs, struct sh_tmu_priv, cs);
+}
+
+static cycle_t sh_tmu_clocksource_read(struct clocksource *cs)
+{
+	struct sh_tmu_priv *p = cs_to_sh_tmu(cs);
+
+	return sh_tmu_read(p, TCNT) ^ 0xffffffff;
+}
+
+static int sh_tmu_clocksource_enable(struct clocksource *cs)
+{
+	struct sh_tmu_priv *p = cs_to_sh_tmu(cs);
+	int ret;
+
+	ret = sh_tmu_enable(p);
+	if (ret)
+		return ret;
+
+	/* TODO: calculate good shift from rate and counter bit width */
+	cs->shift = 10;
+	cs->mult = clocksource_hz2mult(p->rate, cs->shift);
+	return 0;
+}
+
+static void sh_tmu_clocksource_disable(struct clocksource *cs)
+{
+	sh_tmu_disable(cs_to_sh_tmu(cs));
+}
+
+static int sh_tmu_register_clocksource(struct sh_tmu_priv *p,
+				       char *name, unsigned long rating)
+{
+	struct clocksource *cs = &p->cs;
+
+	cs->name = name;
+	cs->rating = rating;
+	cs->read = sh_tmu_clocksource_read;
+	cs->enable = sh_tmu_clocksource_enable;
+	cs->disable = sh_tmu_clocksource_disable;
+	cs->mask = CLOCKSOURCE_MASK(32);
+	cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
+	pr_info("sh_tmu: %s used as clock source\n", cs->name);
+	clocksource_register(cs);
+	return 0;
+}
+
+static struct sh_tmu_priv *ced_to_sh_tmu(struct clock_event_device *ced)
+{
+	return container_of(ced, struct sh_tmu_priv, ced);
+}
+
+static void sh_tmu_clock_event_start(struct sh_tmu_priv *p, int periodic)
+{
+	struct clock_event_device *ced = &p->ced;
+
+	sh_tmu_enable(p);
+
+	/* TODO: calculate good shift from rate and counter bit width */
+
+	ced->shift = 32;
+	ced->mult = div_sc(p->rate, NSEC_PER_SEC, ced->shift);
+	ced->max_delta_ns = clockevent_delta2ns(0xffffffff, ced);
+	ced->min_delta_ns = 5000;
+
+	if (periodic) {
+		p->periodic = (p->rate + HZ/2) / HZ;
+		sh_tmu_set_next(p, p->periodic, 1);
+	}
+}
+
+static void sh_tmu_clock_event_mode(enum clock_event_mode mode,
+				    struct clock_event_device *ced)
+{
+	struct sh_tmu_priv *p = ced_to_sh_tmu(ced);
+	int disabled = 0;
+
+	/* deal with old setting first */
+	switch (ced->mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+	case CLOCK_EVT_MODE_ONESHOT:
+		sh_tmu_disable(p);
+		disabled = 1;
+		break;
+	default:
+		break;
+	}
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+		pr_info("sh_tmu: %s used for periodic clock events\n",
+			ced->name);
+		sh_tmu_clock_event_start(p, 1);
+		break;
+	case CLOCK_EVT_MODE_ONESHOT:
+		pr_info("sh_tmu: %s used for oneshot clock events\n",
+			ced->name);
+		sh_tmu_clock_event_start(p, 0);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+		if (!disabled)
+			sh_tmu_disable(p);
+		break;
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		break;
+	}
+}
+
+static int sh_tmu_clock_event_next(unsigned long delta,
+				   struct clock_event_device *ced)
+{
+	struct sh_tmu_priv *p = ced_to_sh_tmu(ced);
+
+	BUG_ON(ced->mode != CLOCK_EVT_MODE_ONESHOT);
+
+	/* program new delta value */
+	sh_tmu_set_next(p, delta, 0);
+	return 0;
+}
+
+static void sh_tmu_register_clockevent(struct sh_tmu_priv *p,
+				       char *name, unsigned long rating)
+{
+	struct clock_event_device *ced = &p->ced;
+	int ret;
+
+	memset(ced, 0, sizeof(*ced));
+
+	ced->name = name;
+	ced->features = CLOCK_EVT_FEAT_PERIODIC;
+	ced->features |= CLOCK_EVT_FEAT_ONESHOT;
+	ced->rating = rating;
+	ced->cpumask = cpumask_of(0);
+	ced->set_next_event = sh_tmu_clock_event_next;
+	ced->set_mode = sh_tmu_clock_event_mode;
+
+	ret = setup_irq(p->irqaction.irq, &p->irqaction);
+	if (ret) {
+		pr_err("sh_tmu: failed to request irq %d\n",
+		       p->irqaction.irq);
+		return;
+	}
+
+	pr_info("sh_tmu: %s used for clock events\n", ced->name);
+	clockevents_register_device(ced);
+}
+
+static int sh_tmu_register(struct sh_tmu_priv *p, char *name,
+		    unsigned long clockevent_rating,
+		    unsigned long clocksource_rating)
+{
+	if (clockevent_rating)
+		sh_tmu_register_clockevent(p, name, clockevent_rating);
+	else if (clocksource_rating)
+		sh_tmu_register_clocksource(p, name, clocksource_rating);
+
+	return 0;
+}
+
+static int sh_tmu_setup(struct sh_tmu_priv *p, struct platform_device *pdev)
+{
+	struct sh_tmu_config *cfg = pdev->dev.platform_data;
+	struct resource *res;
+	int irq, ret;
+	ret = -ENXIO;
+
+	memset(p, 0, sizeof(*p));
+	p->pdev = pdev;
+
+	if (!cfg) {
+		dev_err(&p->pdev->dev, "missing platform data\n");
+		goto err0;
+	}
+
+	platform_set_drvdata(pdev, p);
+
+	res = platform_get_resource(p->pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&p->pdev->dev, "failed to get I/O memory\n");
+		goto err0;
+	}
+
+	irq = platform_get_irq(p->pdev, 0);
+	if (irq < 0) {
+		dev_err(&p->pdev->dev, "failed to get irq\n");
+		goto err0;
+	}
+
+	/* map memory, let mapbase point to our channel */
+	p->mapbase = ioremap_nocache(res->start, resource_size(res));
+	if (p->mapbase = NULL) {
+		pr_err("sh_tmu: failed to remap I/O memory\n");
+		goto err0;
+	}
+
+	/* setup data for setup_irq() (too early for request_irq()) */
+	p->irqaction.name = cfg->name;
+	p->irqaction.handler = sh_tmu_interrupt;
+	p->irqaction.dev_id = p;
+	p->irqaction.irq = irq;
+	p->irqaction.flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL;
+	p->irqaction.mask = CPU_MASK_NONE;
+
+	/* get hold of clock */
+	p->clk = clk_get(&p->pdev->dev, cfg->clk);
+	if (IS_ERR(p->clk)) {
+		pr_err("sh_tmu: cannot get clock \"%s\"\n", cfg->clk);
+		ret = PTR_ERR(p->clk);
+		goto err1;
+	}
+
+	return sh_tmu_register(p, cfg->name,
+			       cfg->clockevent_rating,
+			       cfg->clocksource_rating);
+ err1:
+	iounmap(p->mapbase);
+ err0:
+	return ret;
+}
+
+static int __devinit sh_tmu_probe(struct platform_device *pdev)
+{
+	struct sh_tmu_priv *p = platform_get_drvdata(pdev);
+	struct sh_tmu_config *cfg = pdev->dev.platform_data;
+	int ret;
+
+	if (p) {
+		pr_info("sh_tmu: %s kept as earlytimer\n", cfg->name);
+		return 0;
+	}
+
+	p = kmalloc(sizeof(*p), GFP_KERNEL);
+	if (p = NULL) {
+		dev_err(&pdev->dev, "failed to allocate driver data\n");
+		return -ENOMEM;
+	}
+
+	ret = sh_tmu_setup(p, pdev);
+	if (ret) {
+		kfree(p);
+		platform_set_drvdata(pdev, NULL);
+	}
+	return ret;
+}
+
+static int __devexit sh_tmu_remove(struct platform_device *pdev)
+{
+	return -EBUSY; /* cannot unregister clockevent and clocksource */
+}
+
+static struct platform_driver sh_tmu_device_driver = {
+	.probe		= sh_tmu_probe,
+	.remove		= __devexit_p(sh_tmu_remove),
+	.driver		= {
+		.name	= "sh_tmu",
+	}
+};
+
+static int __init sh_tmu_init(void)
+{
+	return platform_driver_register(&sh_tmu_device_driver);
+}
+
+static void __exit sh_tmu_exit(void)
+{
+	platform_driver_unregister(&sh_tmu_device_driver);
+}
+
+early_platform_init("earlytimer", &sh_tmu_device_driver);
+module_init(sh_tmu_init);
+module_exit(sh_tmu_exit);
+
+MODULE_AUTHOR("Magnus Damm");
+MODULE_DESCRIPTION("SuperH TMU Timer Driver");
+MODULE_LICENSE("GPL v2");
--- /dev/null
+++ work/include/linux/sh_tmu.h	2009-05-01 13:17:38.000000000 +0900
@@ -0,0 +1,13 @@
+#ifndef __SH_TMU_H__
+#define __SH_TMU_H__
+
+struct sh_tmu_config {
+	char *name;
+	unsigned long channel_offset;
+	int timer_bit;
+	char *clk;
+	unsigned long clockevent_rating;
+	unsigned long clocksource_rating;
+};
+
+#endif /* __SH_TMU_H__ */

Re: [PATCH] clocksource: SuperH TMU Timer driver

[Shin-ichiro KAWASAKI]

Hi Magnus, could you provide an answer to my question on SH-TMU?

Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> This patch adds a TMU driver for the SuperH architecture.
> 
> The TMU driver is a platform driver with early platform
> support to allow using a TMU channel as clockevent or
> clocksource during system bootup or later.
> 
> Clocksource or clockevent can be selected.
> Both periodic and oneshot clockevents are supported.
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  Tested on a Migo-R board with sh7722.
>  Applies on top of the MTU2 patches.
>  Clocksource operation requires the following patch:
>  "clocksource: setup mult_orig in clocksource_enable()"
> 
>  arch/sh/Kconfig              |   12 +
>  drivers/clocksource/Makefile |    1 
>  drivers/clocksource/sh_tmu.c |  461 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sh_tmu.h       |   13 +
>  4 files changed, 487 insertions(+)
> 
> --- 0006/arch/sh/Kconfig
> +++ work/arch/sh/Kconfig	2009-05-01 13:18:03.000000000 +0900
> @@ -116,6 +116,9 @@ config SYS_SUPPORTS_CMT
>  config SYS_SUPPORTS_MTU2
>  	bool
>  
> +config SYS_SUPPORTS_TMU
> +	bool
> +
>  config STACKTRACE_SUPPORT
>  	def_bool y
>  
> @@ -470,6 +473,15 @@ config SH_TMU
>  	help
>  	  This enables the use of the TMU as the system timer.
>  
> +config SH_TIMER_TMU
> +	bool "TMU timer driver"
> +	depends on !SH_TMU && SYS_SUPPORTS_TMU
> +	default y
> +	select GENERIC_TIME
> +	select GENERIC_CLOCKEVENTS
> +	help
> +	  This enables the build of the TMU timer driver.
> +
>  config SH_TIMER_CMT
>  	bool "CMT timer driver"
>  	depends on SYS_SUPPORTS_CMT
> --- 0004/drivers/clocksource/Makefile
> +++ work/drivers/clocksource/Makefile	2009-05-01 13:17:38.000000000 +0900
> @@ -4,3 +4,4 @@ obj-$(CONFIG_X86_PM_TIMER)	+= acpi_pm.o
>  obj-$(CONFIG_SCx200HR_TIMER)	+= scx200_hrt.o
>  obj-$(CONFIG_SH_TIMER_CMT)	+= sh_cmt.o
>  obj-$(CONFIG_SH_TIMER_MTU2)	+= sh_mtu2.o
> +obj-$(CONFIG_SH_TIMER_TMU)	+= sh_tmu.o
> --- /dev/null
> +++ work/drivers/clocksource/sh_tmu.c	2009-05-01 13:20:00.000000000 +0900
> @@ -0,0 +1,461 @@
(snip...)

> +static void sh_tmu_set_next(struct sh_tmu_priv *p, unsigned long delta,
> +			    int periodic)
> +{
> +	/* stop timer */
> +	sh_tmu_start_stop_ch(p, 0);
> +
> +	/* acknowledge interrupt */
> +	sh_tmu_read(p, TCR);
> +
> +	/* enable interrupt */
> +	sh_tmu_write(p, TCR, 0x0020);
> +
> +	/* reload delta value in case of periodic timer */
> +	if (periodic)
> +		sh_tmu_write(p, TCOR, delta);
> +	else
> +		sh_tmu_write(p, TCOR, 0);
> +
> +	sh_tmu_write(p, TCNT, delta);
> +
> +	/* start timer */
> +	sh_tmu_start_stop_ch(p, 1);
> +}

In some occasions, qemu-sh prints out a lot of warnings on timer
with this message : 'Timer with period zero. disabling.'

I made some investigation, and found that the warnings are printed
after zero value is set to TCOR.  I checked SH7785 hardware manual,
but could not find any description what happens when zero value set
to TCOR.

According to the code above, I guess that TMU works as a non-periodic
one-shot timer when the TCOR value is zero. Is it right?


Regards,
Shin-ichiro KAWASAKI

Re: [PATCH] clocksource: SuperH TMU Timer driver

[Magnus Damm]

Hi Kawasaki-san,

2009/5/30 Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp>:
> Hi Magnus, could you provide an answer to my question on SH-TMU?
>
> Magnus Damm wrote:
>> From: Magnus Damm <damm@igel.co.jp>
>>
>> This patch adds a TMU driver for the SuperH architecture.

>> +     /* reload delta value in case of periodic timer */
>> +     if (periodic)
>> +             sh_tmu_write(p, TCOR, delta);
>> +     else
>> +             sh_tmu_write(p, TCOR, 0);

> In some occasions, qemu-sh prints out a lot of warnings on timer
> with this message : 'Timer with period zero. disabling.'
>
> I made some investigation, and found that the warnings are printed
> after zero value is set to TCOR.  I checked SH7785 hardware manual,
> but could not find any description what happens when zero value set
> to TCOR.

Thanks for investigating. I read a few SH manuals before converting
the old driver, but I couldn't find any description of zero value.
Same as you.

The idea with zero value TCOR comes from the old driver. It may be
there for some historical reason, or it may just be plain wrong. I've
sometimes seen interrupt bursts with the old tmu driver which may be
related.

> According to the code above, I guess that TMU works as a non-periodic
> one-shot timer when the TCOR value is zero. Is it right?

Yes, you are correct.

Maybe we should disable the timer channel in a different way. I'm open
to suggestions. =)

Cheers,

/ magnus

Re: [PATCH] clocksource: SuperH TMU Timer driver

[Paul Mundt]

On Mon, Jun 01, 2009 at 07:50:40PM +0900, Magnus Damm wrote:
> 2009/5/30 Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp>:
> > I made some investigation, and found that the warnings are printed
> > after zero value is set to TCOR. ?I checked SH7785 hardware manual,
> > but could not find any description what happens when zero value set
> > to TCOR.
> 
> Thanks for investigating. I read a few SH manuals before converting
> the old driver, but I couldn't find any description of zero value.
> Same as you.
> 
> The idea with zero value TCOR comes from the old driver. It may be
> there for some historical reason, or it may just be plain wrong. I've
> sometimes seen interrupt bursts with the old tmu driver which may be
> related.
> 
It was mostly there just because it worked and seemed like a reasonable
thing to do, and because no one had any better ideas on what to do in
that situation. But yes, technically the behaviour is undefined, and so
finding something slightly more precise is a good idea.

Re: [PATCH] clocksource: SuperH TMU Timer driver

[Shin-ichiro KAWASAKI]

Paul Mundt wrote:
> On Mon, Jun 01, 2009 at 07:50:40PM +0900, Magnus Damm wrote:
>> 2009/5/30 Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp>:
>>> I made some investigation, and found that the warnings are printed
>>> after zero value is set to TCOR. ?I checked SH7785 hardware manual,
>>> but could not find any description what happens when zero value set
>>> to TCOR.
>> Thanks for investigating. I read a few SH manuals before converting
>> the old driver, but I couldn't find any description of zero value.
>> Same as you.
>>
>> The idea with zero value TCOR comes from the old driver. It may be
>> there for some historical reason, or it may just be plain wrong. I've
>> sometimes seen interrupt bursts with the old tmu driver which may be
>> related.
>>
> It was mostly there just because it worked and seemed like a reasonable
> thing to do, and because no one had any better ideas on what to do in
> that situation. But yes, technically the behaviour is undefined, and so
> finding something slightly more precise is a good idea.

Thank you for your explanations, Magnus, Paul!
It has got clearer for me.

I guess there are two solutions.

 i)  Add one-shot timer feature to qemu-sh's TMU emulation.
     (Even though this feature is not documented.)
 ii) Modify sh-linux TMU driver not to set zero value to TCOR.
     (A status will be added to sh_tmu_priv.)

I'll consider on both of them, but that will take a week or so.
Any help from others will be appreciated.


Regards,
Shin-ichiro KAWASAKI

Re: [PATCH] clocksource: SuperH TMU Timer driver

[Shin-ichiro KAWASAKI]

Shin-ichiro KAWASAKI wrote:
> Paul Mundt wrote:
>> On Mon, Jun 01, 2009 at 07:50:40PM +0900, Magnus Damm wrote:
>>> 2009/5/30 Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp>:
>>>> I made some investigation, and found that the warnings are printed
>>>> after zero value is set to TCOR. ?I checked SH7785 hardware manual,
>>>> but could not find any description what happens when zero value set
>>>> to TCOR.
>>> Thanks for investigating. I read a few SH manuals before converting
>>> the old driver, but I couldn't find any description of zero value.
>>> Same as you.
>>>
>>> The idea with zero value TCOR comes from the old driver. It may be
>>> there for some historical reason, or it may just be plain wrong. I've
>>> sometimes seen interrupt bursts with the old tmu driver which may be
>>> related.
>>>
>> It was mostly there just because it worked and seemed like a reasonable
>> thing to do, and because no one had any better ideas on what to do in
>> that situation. But yes, technically the behaviour is undefined, and so
>> finding something slightly more precise is a good idea.
> 
> Thank you for your explanations, Magnus, Paul!
> It has got clearer for me.
> 
> I guess there are two solutions.
> 
> i)  Add one-shot timer feature to qemu-sh's TMU emulation.
>     (Even though this feature is not documented.)
> ii) Modify sh-linux TMU driver not to set zero value to TCOR.
>     (A status will be added to sh_tmu_priv.)
> 
> I'll consider on both of them, but that will take a week or so.
> Any help from others will be appreciated.

Here's the patch which suggests TMU driver modification.

It does,
- Sets maximum value 0xffffffff to TCOR instead of zero.
  Then after one shot interrupt, next count down runs
  for a while (around seconds?)
- Stops the timer in the one shot interrupt handler.
  Then next count down does not reach to zero.

This patch assumes that while TMU counting down from 0xffffffff,
the interrupt handler finishes the task to stop the timer.

I confirmed that the timer warnings from qemu-sh disappear
with this patch.  No test is done with real hardware.

I'm not familiar with linux clock framework.
Any comments are welcome.

Regards,
Shin-ichiro KAWASAKI


diff --git a/drivers/clocksource/sh_tmu.c b/drivers/clocksource/sh_tmu.c
index d6ea439..7bb0bd3 100644
--- a/drivers/clocksource/sh_tmu.c
+++ b/drivers/clocksource/sh_tmu.c
@@ -158,7 +158,7 @@ static void sh_tmu_set_next(struct sh_tmu_priv *p, unsigned long delta,
	if (periodic)
		sh_tmu_write(p, TCOR, delta);
	else
-		sh_tmu_write(p, TCOR, 0);
+		sh_tmu_write(p, TCOR, 0xffffffff);

	sh_tmu_write(p, TCNT, delta);

@@ -171,9 +171,10 @@ static irqreturn_t sh_tmu_interrupt(int irq, void *dev_id)
	struct sh_tmu_priv *p = dev_id;

	/* disable or acknowledge interrupt */
-	if (p->ced.mode = CLOCK_EVT_MODE_ONESHOT)
+	if (p->ced.mode = CLOCK_EVT_MODE_ONESHOT) {
+		sh_tmu_start_stop_ch(p, 0);
		sh_tmu_write(p, TCR, 0x0000);
-	else
+	} else
		sh_tmu_write(p, TCR, 0x0020);

	/* notify clockevent layer */