[PATCH] m68k/mvme16x: Fix timer interrupts

[PATCH] m68k/mvme16x: Fix timer interrupts

[Mike Pavone]

Timer interrupts on MVME16x and MVME17x boards are broken as the CEN and
COC bits are being inadvertently cleared when clearing the overflow
counter. This results in no timer interrupts being delivered after the
first. Initialization then hangs in calibrate_delay as the jiffies counter
is not updated. OR with current register value to preserve these bits.

Fixes: 19999a8b8782 ("m68k: mvme16x: Handle timer counter overflow")
Signed-off-by: Michael Pavone <pavone@retrodev.com>
---
  arch/m68k/mvme16x/config.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/m68k/mvme16x/config.c b/arch/m68k/mvme16x/config.c
index 30357fe4ba6c..949d3e19f79c 100644
--- a/arch/m68k/mvme16x/config.c
+++ b/arch/m68k/mvme16x/config.c
@@ -375,7 +375,7 @@ static irqreturn_t mvme16x_timer_int (int irq, void *dev_id)
  
  	local_irq_save(flags);
  	out_8(PCCTIC1, in_8(PCCTIC1) | PCCTIC1_INT_CLR);
-	out_8(PCCTOVR1, PCCTOVR1_OVR_CLR);
+	out_8(PCCTOVR1, in_8(PCCTOVR1) | PCCTOVR1_OVR_CLR);
  	clk_total += PCC_TIMER_CYCLES;
  	legacy_timer_tick(1);
  	local_irq_restore(flags);

Re: [PATCH] m68k/mvme16x: Fix timer interrupts

[Finn Thain]

On Tue, 16 Mar 2021, Mike Pavone wrote:

> Timer interrupts on MVME16x and MVME17x boards are broken as the CEN and
> COC bits are being inadvertently cleared when clearing the overflow
> counter. This results in no timer interrupts being delivered after the
> first. Initialization then hangs in calibrate_delay as the jiffies counter
> is not updated. OR with current register value to preserve these bits.
> 
> Fixes: 19999a8b8782 ("m68k: mvme16x: Handle timer counter overflow")
> Signed-off-by: Michael Pavone <pavone@retrodev.com>

Sorry about that regression.

Do you think that commit 7529b90d051e ("m68k: mvme147: Handle timer 
counter overflow") has the same problem?

> ---
>  arch/m68k/mvme16x/config.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/m68k/mvme16x/config.c b/arch/m68k/mvme16x/config.c
> index 30357fe4ba6c..949d3e19f79c 100644
> --- a/arch/m68k/mvme16x/config.c
> +++ b/arch/m68k/mvme16x/config.c
> @@ -375,7 +375,7 @@ static irqreturn_t mvme16x_timer_int (int irq, void
> *dev_id)
>   	local_irq_save(flags);
>  	out_8(PCCTIC1, in_8(PCCTIC1) | PCCTIC1_INT_CLR);
> -	out_8(PCCTOVR1, PCCTOVR1_OVR_CLR);
> +	out_8(PCCTOVR1, in_8(PCCTOVR1) | PCCTOVR1_OVR_CLR);
>  	clk_total += PCC_TIMER_CYCLES;
>  	legacy_timer_tick(1);
>  	local_irq_restore(flags);
> 
> 

Re: [PATCH] m68k/mvme16x: Fix timer interrupts

[Michael Pavone]

On 3/17/21 8:05 PM, Finn Thain wrote:

> On Tue, 16 Mar 2021, Mike Pavone wrote:
>
>> Timer interrupts on MVME16x and MVME17x boards are broken as the CEN and
>> COC bits are being inadvertently cleared when clearing the overflow
>> counter. This results in no timer interrupts being delivered after the
>> first. Initialization then hangs in calibrate_delay as the jiffies counter
>> is not updated. OR with current register value to preserve these bits.
>>
>> Fixes: 19999a8b8782 ("m68k: mvme16x: Handle timer counter overflow")
>> Signed-off-by: Michael Pavone <pavone@retrodev.com>
> Sorry about that regression.
>
> Do you think that commit 7529b90d051e ("m68k: mvme147: Handle timer
> counter overflow") has the same problem?

I don't have an MVME147 board to test with, but based on the documentation
it looks like it does. The clear bit is in the same register as the enable
and count enable bits on the original PCCchip.

Re: [PATCH] m68k/mvme16x: Fix timer interrupts

[Finn Thain]

On Thu, 18 Mar 2021, Michael Pavone wrote:

> On 3/17/21 8:05 PM, Finn Thain wrote:
> 
> > On Tue, 16 Mar 2021, Mike Pavone wrote:
> >
> >> Timer interrupts on MVME16x and MVME17x boards are broken as the CEN and
> >> COC bits are being inadvertently cleared when clearing the overflow
> >> counter. This results in no timer interrupts being delivered after the
> >> first. Initialization then hangs in calibrate_delay as the jiffies counter
> >> is not updated. OR with current register value to preserve these bits.
> >>
> >> Fixes: 19999a8b8782 ("m68k: mvme16x: Handle timer counter overflow")
> >> Signed-off-by: Michael Pavone <pavone@retrodev.com>
> > Sorry about that regression.
> >
> > Do you think that commit 7529b90d051e ("m68k: mvme147: Handle timer 
> > counter overflow") has the same problem?
> 
> I don't have an MVME147 board to test with, but based on the 
> documentation it looks like it does. The clear bit is in the same 
> register as the enable and count enable bits on the original PCCchip.
> 

OK. I will write a patch for mvme147.

BTW, assuming you did get the kernel running on your hardware, do you know 
if there's any need for the '#warning check me!' that still remains in 
arch/m68k/mvme16x/config.c and arch/m68k/mvme147/config.c?

Re: [PATCH] m68k/mvme16x: Fix timer interrupts

[Finn Thain]

On Tue, 16 Mar 2021, Mike Pavone wrote:

> Timer interrupts on MVME16x and MVME17x boards are broken as the CEN and
> COC bits are being inadvertently cleared when clearing the overflow
> counter. This results in no timer interrupts being delivered after the
> first. Initialization then hangs in calibrate_delay as the jiffies counter
> is not updated. OR with current register value to preserve these bits.
> 
> Fixes: 19999a8b8782 ("m68k: mvme16x: Handle timer counter overflow")
> Signed-off-by: Michael Pavone <pavone@retrodev.com>
> ---
>  arch/m68k/mvme16x/config.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/m68k/mvme16x/config.c b/arch/m68k/mvme16x/config.c
> index 30357fe4ba6c..949d3e19f79c 100644
> --- a/arch/m68k/mvme16x/config.c
> +++ b/arch/m68k/mvme16x/config.c
> @@ -375,7 +375,7 @@ static irqreturn_t mvme16x_timer_int (int irq, void
> *dev_id)
>   	local_irq_save(flags);
>  	out_8(PCCTIC1, in_8(PCCTIC1) | PCCTIC1_INT_CLR);
> -	out_8(PCCTOVR1, PCCTOVR1_OVR_CLR);
> +	out_8(PCCTOVR1, in_8(PCCTOVR1) | PCCTOVR1_OVR_CLR);

Since we know at compile time what bits should be set in PCCTOVR1, we can 
avoid the additional register access. We should do the same in 
mvme16x_sched_init() for clarity. What do you think about this patch?

diff --git a/arch/m68k/include/asm/mvme147hw.h b/arch/m68k/include/asm/mvme147hw.h
index 257b29184af9..e28eb1c0e0bf 100644
--- a/arch/m68k/include/asm/mvme147hw.h
+++ b/arch/m68k/include/asm/mvme147hw.h
@@ -66,6 +66,9 @@ struct pcc_regs {
 #define PCC_INT_ENAB		0x08
 
 #define PCC_TIMER_INT_CLR	0x80
+
+#define PCC_TIMER_TIC_EN	0x01
+#define PCC_TIMER_COC_EN	0x02
 #define PCC_TIMER_CLR_OVF	0x04
 
 #define PCC_LEVEL_ABORT		0x07
diff --git a/arch/m68k/mvme147/config.c b/arch/m68k/mvme147/config.c
index cfdc7f912e14..c16936a4c883 100644
--- a/arch/m68k/mvme147/config.c
+++ b/arch/m68k/mvme147/config.c
@@ -115,7 +115,8 @@ static irqreturn_t mvme147_timer_int (int irq, void *dev_id)
 
 	local_irq_save(flags);
 	m147_pcc->t1_int_cntrl = PCC_TIMER_INT_CLR;
-	m147_pcc->t1_cntrl = PCC_TIMER_CLR_OVF;
+	m147_pcc->t1_cntrl = PCC_TIMER_CLR_OVF | PCC_TIMER_COC_EN |
+			     PCC_TIMER_TIC_EN;
 	clk_total += PCC_TIMER_CYCLES;
 	legacy_timer_tick(1);
 	local_irq_restore(flags);
@@ -133,8 +134,7 @@ void mvme147_sched_init (void)
 	/* Init the clock with a value */
 	/* The clock counter increments until 0xFFFF then reloads */
 	m147_pcc->t1_preload = PCC_TIMER_PRELOAD;
-	m147_pcc->t1_cntrl = 0x0;	/* clear timer */
-	m147_pcc->t1_cntrl = 0x3;	/* start timer */
+	m147_pcc->t1_cntrl = PCC_TIMER_COC_EN | PCC_TIMER_TIC_EN;
 	m147_pcc->t1_int_cntrl = PCC_TIMER_INT_CLR;  /* clear pending ints */
 	m147_pcc->t1_int_cntrl = PCC_INT_ENAB|PCC_LEVEL_TIMER1;
 
diff --git a/arch/m68k/mvme16x/config.c b/arch/m68k/mvme16x/config.c
index 30357fe4ba6c..9833528f9981 100644
--- a/arch/m68k/mvme16x/config.c
+++ b/arch/m68k/mvme16x/config.c
@@ -375,7 +375,7 @@ static irqreturn_t mvme16x_timer_int (int irq, void *dev_id)
 
 	local_irq_save(flags);
 	out_8(PCCTIC1, in_8(PCCTIC1) | PCCTIC1_INT_CLR);
-	out_8(PCCTOVR1, PCCTOVR1_OVR_CLR);
+	out_8(PCCTOVR1, PCCTOVR1_OVR_CLR | PCCTOVR1_TIC_EN | PCCTOVR1_COC_EN);
 	clk_total += PCC_TIMER_CYCLES;
 	legacy_timer_tick(1);
 	local_irq_restore(flags);
@@ -391,7 +391,7 @@ void mvme16x_sched_init(void)
     /* Using PCCchip2 or MC2 chip tick timer 1 */
     out_be32(PCCTCNT1, 0);
     out_be32(PCCTCMP1, PCC_TIMER_CYCLES);
-    out_8(PCCTOVR1, in_8(PCCTOVR1) | PCCTOVR1_TIC_EN | PCCTOVR1_COC_EN);
+    out_8(PCCTOVR1, PCCTOVR1_TIC_EN | PCCTOVR1_COC_EN);
     out_8(PCCTIC1, PCCTIC1_INT_EN | 6);
     if (request_irq(MVME16x_IRQ_TIMER, mvme16x_timer_int, IRQF_TIMER, "timer",
                     NULL))

Re: [PATCH] m68k/mvme16x: Fix timer interrupts

[Michael Pavone]

On 3/19/21 12:07 AM, Finn Thain wrote:

> Since we know at compile time what bits should be set in PCCTOVR1, we can
> avoid the additional register access. We should do the same in
> mvme16x_sched_init() for clarity. What do you think about this patch?

Makes sense and your patch works fine on my MVME177.

On 3/18/21 11:35 PM, Finn Thain wrote:
> BTW, assuming you did get the kernel running on your hardware, do you know 
> if there's any need for the '#warning check me!' that still remains in 
> arch/m68k/mvme16x/config.c and arch/m68k/mvme147/config.c?

It's running in the sense that it successfully runs init, but the lack of
a proper tty driver makes it a bit painful to check anything else. I'm
going to take a stab at writing a new one next.