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
