Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 12 Nov 2018 10:37:28 -0800 (PST)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Finn Thain <fthain@telegraphics.com.au>
cc:     Geert Uytterhoeven <geert@linux-m68k.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Stephen N Chivers <schivers@csc.com.au>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        John Stultz <john.stultz@linaro.org>,
        linux-m68k@lists.linux-m68k.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 13/13] m68k: mvme16x: Convert to clocksource API
In-Reply-To: <2f4015ba435f6f06b874295d2a47319875474c7f.1541995959.git.fthain@telegraphics.com.au>
Message-ID: <alpine.DEB.2.21.1811121001140.14703@nanos.tec.linutronix.de>
References: <cover.1541995959.git.fthain@telegraphics.com.au> <2f4015ba435f6f06b874295d2a47319875474c7f.1541995959.git.fthain@telegraphics.com.au>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Finn,

On Mon, 12 Nov 2018, Finn Thain wrote:

First of all, thanks for tackling this!

> +static u32 clk_total;
> +
> +#define PCC_TIMER_CLOCK_FREQ 1000000
> +#define PCC_TIMER_CYCLES     (PCC_TIMER_CLOCK_FREQ / HZ)
> +
>  static irqreturn_t mvme16x_timer_int (int irq, void *dev_id)
>  {
> +    irq_handler_t tick_handler = dev_id;
> +    unsigned long flags;
> +
> +    local_irq_save(flags);

No need for local_irq_save() here. Interrupt handlers are guaranteed to be
called with interrupts disabled.

>      *(volatile unsigned char *)0xfff4201b |= 8;
> +    clk_total += PCC_TIMER_CYCLES;

Looking at the read function below, I assume that this magic 0xfff42008
register counts up to PCC_TIMER_CYCLES, which triggers the timer interrupt
and then starts from 0 again. And looking at the programming manual
actually confirms that assumption (COC is set in the control reg).

> -u32 mvme16x_gettimeoffset(void)
> +static u64 mvme16x_read_clk(struct clocksource *cs)
>  {
> -    return (*(volatile u32 *)0xfff42008) * 1000;
> +    u32 count = *(volatile u32 *)0xfff42008;
> +
> +    return clk_total + count;
>  }

There is a problem with that approach. Assume the following situation:

				Counter value (HZ = 100)
       local_irq_disable()	
       ...
       ktime_get()		9999
       ....			10000 -> 0 (interrupt is triggered)
       ktime_get()		1

IOW, time goes backwards.

There are two ways to solve that:

 1) Take the overflow bits in the timer control register into account,
    which makes time readout even slower because you need to do it like
    this:

    do {
       ovfl = read(TCR1);
       now = read(TCNT1);
    while (ovfl != read(TCR1));
    ....
  

 2) Use Timer2 in freerunning mode which means it will use the full 32bit
    and then wrap back to 0. That's fine and the clocksource core handles
    that.

    That removes the clk_total thing and just works.

Thanks,

	tglx