lkml.org 
[lkml]   [2018]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH 13/13] m68k: mvme16x: Convert to clocksource API
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
>

\
 
 \ /
  Last update: 2018-11-13 01:12    [W:0.087 / U:2.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site