LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Finn Thain <fthain@telegraphics.com.au>
To: Thomas Gleixner <tglx@linutronix.de>
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
Date: Tue, 13 Nov 2018 11:11:33 +1100 (AEDT)
Message-ID: <alpine.LNX.2.21.1811130826260.356@nippy.intranet> (raw)
In-Reply-To: <alpine.DEB.2.21.1811121001140.14703@nanos.tec.linutronix.de>

On Mon, 12 Nov 2018, Thomas Gleixner wrote:

> Finn,
> 
> First of all, thanks for tackling this!
> 

Happy to help. Thanks for your review.

> > +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.
> 

That's not the case on m68k, as I understand it. However, the CPU 
interrupt level does prevent interrupt handlers from nesting.

> >      *(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.
> 

Yes. It's not a new problem. I think hp300 and mvme147 have similar 
issues.

If the clocksource core is badly affected by this problem then I'll have 
to address it.

> 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));
>     ....
>   

How about,

	local_irq_save(flags);
	ovfl = read(TCR1);
	now = read(TCNT1);
	if (ovfl != read(TCR1))
		now = read(TCNT1);

	ticks = clk_total + now;
	offset = (ovfl >> 4) * PCC_TIMER_CYCLES;
	local_irq_restore(flags);

	return offset + ticks;

This has to be synchronized with the interrupt handler because the 
interrupt handler must clear the overflow counter in TCR1 when it does 
clk_total += PCC_TIMER_CYCLES;

BTW, there are some synchronization bugs in this patch series that will be 
addressed in the next submission. They have been fixed in my github repo.

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

I really like this suggestion!

But I fear it is a change that cannot be checked by inspection. If I wrote 
it, I'd want to test it, or have someone else test it. (Stephen?)

I wonder if there exists an emulator for MVME 162/166/167/172/177 machines 
capable of booting Linux...

-- 

> Thanks,
> 
> 	tglx
> 

  reply index

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-12  4:12 [RFC PATCH 00/13] m68k: Drop arch_gettimeoffset and adopt " Finn Thain
2018-11-12  4:12 ` [RFC PATCH 08/13] m68k: atari: Convert to " Finn Thain
2018-11-12  4:12 ` [RFC PATCH 04/13] m68k: mac: Clean up unused timer definitions Finn Thain
2018-11-12  4:12 ` [RFC PATCH 10/13] m68k: hp300: Convert to clocksource API Finn Thain
2018-11-12  4:12 ` [RFC PATCH 11/13] m68k: mac: " Finn Thain
2018-11-12  4:12 ` [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset Finn Thain
2018-11-12  8:34   ` Christoph Hellwig
2018-11-13  3:39     ` Finn Thain
2018-11-13  9:20       ` Russell King - ARM Linux
2018-11-13 21:55         ` Finn Thain
2018-11-13 23:43           ` Russell King - ARM Linux
2018-11-14  1:35             ` Michael Schmitz
2018-11-14  7:58               ` Russell King - ARM Linux
2018-11-15  1:34                 ` Michael Schmitz
2018-11-14  3:17             ` Finn Thain
2018-11-14 14:16               ` Russell King - ARM Linux
2018-11-14 14:58                 ` Geert Uytterhoeven
2018-11-14 18:11                   ` Russell King - ARM Linux
2018-11-15  4:12                 ` Finn Thain
2018-11-16 17:47                   ` Russell King - ARM Linux
2018-11-16 22:49                     ` Finn Thain
2018-11-17  3:00                       ` Michael Schmitz
2018-11-14 12:05   ` Russell King - ARM Linux
2018-11-12  4:12 ` [RFC PATCH 02/13] m68k: " Finn Thain
2018-11-12  4:12 ` [RFC PATCH 07/13] m68k: amiga: Convert to clocksource API Finn Thain
2018-11-12  9:02   ` Geert Uytterhoeven
2018-11-12  9:21     ` Finn Thain
2018-11-12  4:12 ` [RFC PATCH 09/13] m68k: bvme6000: " Finn Thain
2018-11-12  4:12 ` [RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET Finn Thain
2018-11-12  9:00   ` Geert Uytterhoeven
2018-11-12  9:06     ` Finn Thain
2018-11-13  2:29       ` Michael Schmitz
2018-11-13  3:14         ` Finn Thain
2018-11-13  4:50           ` Michael Schmitz
2018-11-13  6:15             ` Finn Thain
2018-11-13  8:24               ` Michael Schmitz
2018-11-13 22:11                 ` Finn Thain
2018-11-14  1:08                   ` Michael Schmitz
2018-11-14  2:58                     ` Michael Schmitz
2018-11-14 23:54                       ` Michael Schmitz
2018-11-15  4:37                         ` Finn Thain
2018-11-15  6:35                         ` Michael Schmitz
2018-11-16  0:04                           ` Finn Thain
2018-11-12  4:12 ` [RFC PATCH 13/13] m68k: mvme16x: Convert to clocksource API Finn Thain
2018-11-12 18:37   ` Thomas Gleixner
2018-11-13  0:11     ` Finn Thain [this message]
2018-11-13 22:04       ` Finn Thain
2018-11-13 22:10         ` [RFC PATCH 13/13] m68k: mvme16x: Convert to clocksource APIy Thomas Gleixner
2018-11-13 22:33           ` Finn Thain
2018-11-18 17:12             ` Thomas Gleixner
2018-11-12  4:12 ` [RFC PATCH 03/13] m68k: mac: Fix VIA timer counter accesses Finn Thain
2018-11-12  4:12 ` [RFC PATCH 12/13] m68k: mvme147: Convert to clocksource API Finn Thain
2018-11-12  4:12 ` [RFC PATCH 05/13] m68k: apollo, q40, sun3, sun3x: Remove arch_gettimeoffset implementations Finn Thain

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LNX.2.21.1811130826260.356@nippy.intranet \
    --to=fthain@telegraphics.com.au \
    --cc=arnd@arndb.de \
    --cc=daniel.lezcano@linaro.org \
    --cc=geert@linux-m68k.org \
    --cc=john.stultz@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-m68k@lists.linux-m68k.org \
    --cc=schivers@csc.com.au \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git