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;
Subject: Re: [RFC PATCH v2 07/14] m68k: atari: Convert to clocksource API
To:     Finn Thain <fthain@telegraphics.com.au>,
        Geert Uytterhoeven <geert@linux-m68k.org>
References: <cover.1542589838.git.fthain@telegraphics.com.au>
 <b94b3aed32c0ae0f5de786ffc746ffb20fbe2d1e.1542589838.git.fthain@telegraphics.com.au>
Cc:     Andreas Schwab <schwab@linux-m68k.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Stephen N Chivers <schivers@csc.com.au>,
        Thomas Gleixner <tglx@linutronix.de>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        John Stultz <john.stultz@linaro.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        linux-m68k@lists.linux-m68k.org, linux-kernel@vger.kernel.org
From:   Michael Schmitz <schmitzmic@gmail.com>
Message-ID: <c465ebbb-69f8-c42c-c2d0-0e57dffe30a5@gmail.com>
Date:   Mon, 19 Nov 2018 21:35:27 +1300
User-Agent: Mozilla/5.0 (X11; Linux ppc; rv:45.0) Gecko/20100101
 Icedove/45.4.0
MIME-Version: 1.0
In-Reply-To: <b94b3aed32c0ae0f5de786ffc746ffb20fbe2d1e.1542589838.git.fthain@telegraphics.com.au>
Content-Type: text/plain; charset=iso-8859-15; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Finn,

this fixes the ssh timeout issues I reported for the earlier versions. 
Can't see a large degradation of resolution either.

The race in the previous versions may have been exacerbated in part by 
an incorrect assumption about the likelihood of counter wrap-around in 
the old atari_gettimeoffset() implementation. The comment in the code 
states that the likelihood is just 2%, so skips the interrupt pending 
bit check if the counter has run down more than 2%. It does not follow 
that the counter never runs down further than that threshold though. I 
have found the distribution of counter values observed with interrupt 
pending to be quite long-tailed, with a 50% threshold just beginning to 
catch the majority of wrap-arounds.

Your solution to stop the clock instead of allowing a jump backwards is 
much safer in this context.

Am 19.11.2018 um 14:10 schrieb Finn Thain:
> Add a platform clocksource by adapting the existing arch_gettimeoffset
> implementation.
>
> Normally the MFP timer C interrupt flag would be used to check for
> timer counter wrap-around. Unfortunately, that flag gets cleared by the
> MFP itself (due to automatic EOI mode). This means that
> mfp_timer_c_handler() and atari_read_clk() must race when accounting
> for counter wrap-around.
>
> That problem is avoided here by effectively stopping the clock when it
> might otherwise jump backwards. This harms clock accuracy; the result
> is not much better than the jiffies clocksource. Also, clock error is
> no longer uniformly distributed.
>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

Tested-by: Michael Schmitz <schmitzmic@gmail.com>
> ---
> TODO: find a spare counter for the clocksource, rather than hanging
> it off the HZ timer.
>
> It would be simpler to adopt the 'jiffies' clocksource here
> (c.f. patch for the hp300 platform in this series).
>
> Changed since v1:
>  - Moved clk_total access to within the irq lock.
>  - Renamed mfp_timer_handler and mfptimer_handler.
>  - Avoid accessing the timer interrupt flag in atari_read_clk(). To
> get monotonicity, keep track of the previous timer counter value.
> ---
>  arch/m68k/atari/time.c | 48 +++++++++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/arch/m68k/atari/time.c b/arch/m68k/atari/time.c
> index fafa20f75ab9..914832e55ec5 100644
> --- a/arch/m68k/atari/time.c
> +++ b/arch/m68k/atari/time.c
> @@ -16,6 +16,7 @@
>  #include <linux/init.h>
>  #include <linux/rtc.h>
>  #include <linux/bcd.h>
> +#include <linux/clocksource.h>
>  #include <linux/delay.h>
>  #include <linux/export.h>
>
> @@ -24,12 +25,27 @@
>  DEFINE_SPINLOCK(rtc_lock);
>  EXPORT_SYMBOL_GPL(rtc_lock);
>
> +static u64 atari_read_clk(struct clocksource *cs);
> +
> +static struct clocksource atari_clk = {
> +	.name   = "mfp",
> +	.rating = 100,
> +	.read   = atari_read_clk,
> +	.mask   = CLOCKSOURCE_MASK(32),
> +	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
> +};
> +
> +static u32 clk_total;
> +static u32 last_timer_count;
> +
>  static irqreturn_t mfp_timer_c_handler(int irq, void *dev_id)
>  {
>  	irq_handler_t timer_routine = dev_id;
>  	unsigned long flags;
>
>  	local_irq_save(flags);
> +	last_timer_count = st_mfp.tim_dt_c;
> +	clk_total += INT_TICKS;
>  	timer_routine(0, NULL);
>  	local_irq_restore(flags);
>
> @@ -44,32 +60,30 @@ atari_sched_init(irq_handler_t timer_routine)
>      /* start timer C, div = 1:100 */
>      st_mfp.tim_ct_cd = (st_mfp.tim_ct_cd & 15) | 0x60;
>      /* install interrupt service routine for MFP Timer C */
> -    if (request_irq(IRQ_MFP_TIMC, mfp_timer_c_handler, 0, "timer",
> +    if (request_irq(IRQ_MFP_TIMC, mfp_timer_c_handler, IRQF_TIMER, "timer",
>                      timer_routine))
>  	pr_err("Couldn't register timer interrupt\n");
> +
> +    clocksource_register_hz(&atari_clk, INT_CLK);
>  }
>
>  /* ++andreas: gettimeoffset fixed to check for pending interrupt */
>
> -#define TICK_SIZE 10000
> -
> -/* This is always executed with interrupts disabled.  */
> -u32 atari_gettimeoffset(void)
> +static u64 atari_read_clk(struct clocksource *cs)
>  {
> -  u32 ticks, offset = 0;
> -
> -  /* read MFP timer C current value */
> -  ticks = st_mfp.tim_dt_c;
> -  /* The probability of underflow is less than 2% */
> -  if (ticks > INT_TICKS - INT_TICKS / 50)
> -    /* Check for pending timer interrupt */
> -    if (st_mfp.int_pn_b & (1 << 5))
> -      offset = TICK_SIZE;
> +	unsigned long flags;
> +	u32 ticks;
>
> -  ticks = INT_TICKS - ticks;
> -  ticks = ticks * 10000L / INT_TICKS;
> +	local_irq_save(flags);
> +	ticks = st_mfp.tim_dt_c;
> +	if (ticks > last_timer_count) /* timer wrapped since last interrupt */
> +		ticks = last_timer_count;
> +	last_timer_count = ticks;
> +	ticks = INT_TICKS - ticks;
> +	ticks += clk_total;
> +	local_irq_restore(flags);
>
> -  return (ticks + offset) * 1000;
> +	return ticks;
>  }
>
>
>