Re: [PATCH 01/16 v3] pmac_zilog: fix unexpected irq

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Wed, 2011-12-07 at 14:49 +1100, Finn Thain wrote:
> On most 68k Macs the SCC IRQ is an autovector interrupt and cannot be 
> masked. This can be a problem when pmac_zilog starts up.

Thanks. I'll test it on a powermac or two and will merge it via the
powerpc -next tree if it works out allright.

Cheers,
Ben.
 

> For example, the serial debugging code in arch/m68k/kernel/head.S may be 
> used beforehand. It disables the SCC interrupts at the chip but doesn't 
> ack them. Then when a pmac_zilog port is used, the machine locks up with 
> "unexpected interrupt".
> 
> This can happen in pmz_shutdown() since the irq is freed before the 
> channel interrupts are disabled.
> 
> Fix this by clearing interrupt enable bits before the handler is 
> uninstalled. Also move the interrupt control bit flipping into a separate 
> pmz_interrupt_control() routine. Replace all instances of these operations 
> with calls to this routine. Omit the zssync() calls that seem to serve no 
> purpose.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> Acked-by: Alan Cox <alan@linux.intel.com>
> 
> ---
>    
> Re-implemented as v2 using a simpler approach that avoids messing with the 
> Master Interrupt Enable bit. As well as the ifdef problem, it turns out 
> that v1 was not sufficient to entirely fix the problem.
> 
> v3 avoids needless changes to the logic and locking in the suspend and 
> shutdown code and tries to keep register writes closer to their original 
> sequence.
> 
> This patch has been tested on a PowerBook 520 but no PowerMacs yet.
> 
> 
> Index: linux-git/drivers/tty/serial/pmac_zilog.c
> ===================================================================
> --- linux-git.orig/drivers/tty/serial/pmac_zilog.c	2011-12-07 12:36:32.000000000 +1100
> +++ linux-git/drivers/tty/serial/pmac_zilog.c	2011-12-07 14:29:21.000000000 +1100
> @@ -216,6 +216,18 @@ static void pmz_maybe_update_regs(struct
>  	}
>  }
>  
> +static void pmz_interrupt_control(struct uart_pmac_port *uap, int enable)
> +{
> +	if (enable) {
> +		uap->curregs[1] |= INT_ALL_Rx | TxINT_ENAB;
> +		if (!ZS_IS_EXTCLK(uap))
> +			uap->curregs[1] |= EXT_INT_ENAB;
> +	} else {
> +		uap->curregs[1] &= ~(EXT_INT_ENAB | TxINT_ENAB | RxINT_MASK);
> +	}
> +	write_zsreg(uap, R1, uap->curregs[1]);
> +}
> +
>  static struct tty_struct *pmz_receive_chars(struct uart_pmac_port *uap)
>  {
>  	struct tty_struct *tty = NULL;
> @@ -339,9 +351,7 @@ static struct tty_struct *pmz_receive_ch
>  
>  	return tty;
>   flood:
> -	uap->curregs[R1] &= ~(EXT_INT_ENAB | TxINT_ENAB | RxINT_MASK);
> -	write_zsreg(uap, R1, uap->curregs[R1]);
> -	zssync(uap);
> +	pmz_interrupt_control(uap, 0);
>  	pmz_error("pmz: rx irq flood !\n");
>  	return tty;
>  }
> @@ -990,12 +1000,9 @@ static int pmz_startup(struct uart_port
>  	if (ZS_IS_IRDA(uap))
>  		pmz_irda_reset(uap);
>  
> -	/* Enable interrupts emission from the chip */
> +	/* Enable interrupt requests for the channel */
>  	spin_lock_irqsave(&port->lock, flags);
> -	uap->curregs[R1] |= INT_ALL_Rx | TxINT_ENAB;
> -	if (!ZS_IS_EXTCLK(uap))
> -		uap->curregs[R1] |= EXT_INT_ENAB;
> -	write_zsreg(uap, R1, uap->curregs[R1]);
> +	pmz_interrupt_control(uap, 1);
>  	spin_unlock_irqrestore(&port->lock, flags);
>  
>  	pmz_debug("pmz: startup() done.\n");
> @@ -1015,6 +1022,25 @@ static void pmz_shutdown(struct uart_por
>  
>  	mutex_lock(&pmz_irq_mutex);
>  
> +	spin_lock_irqsave(&port->lock, flags);
> +
> +	if (!ZS_IS_ASLEEP(uap)) {
> +		/* Disable interrupt requests for the channel */
> +		pmz_interrupt_control(uap, 0);
> +
> +		if (!ZS_IS_CONS(uap)) {
> +			/* Disable receiver and transmitter */
> +			uap->curregs[R3] &= ~RxENABLE;
> +			uap->curregs[R5] &= ~TxENABLE;
> +
> +			/* Disable break assertion */
> +			uap->curregs[R5] &= ~SND_BRK;
> +			pmz_maybe_update_regs(uap);
> +		}
> +	}
> +
> +	spin_unlock_irqrestore(&port->lock, flags);
> +
>  	/* Release interrupt handler */
>  	free_irq(uap->port.irq, uap);
>  
> @@ -1025,29 +1051,8 @@ static void pmz_shutdown(struct uart_por
>  	if (!ZS_IS_OPEN(uap->mate))
>  		pmz_get_port_A(uap)->flags &= ~PMACZILOG_FLAG_IS_IRQ_ON;
>  
> -	/* Disable interrupts */
> -	if (!ZS_IS_ASLEEP(uap)) {
> -		uap->curregs[R1] &= ~(EXT_INT_ENAB | TxINT_ENAB | RxINT_MASK);
> -		write_zsreg(uap, R1, uap->curregs[R1]);
> -		zssync(uap);
> -	}
> -
> -	if (ZS_IS_CONS(uap) || ZS_IS_ASLEEP(uap)) {
> -		spin_unlock_irqrestore(&port->lock, flags);
> -		mutex_unlock(&pmz_irq_mutex);
> -		return;
> -	}
> -
> -	/* Disable receiver and transmitter.  */
> -	uap->curregs[R3] &= ~RxENABLE;
> -	uap->curregs[R5] &= ~TxENABLE;
> -
> -	/* Disable all interrupts and BRK assertion.  */
> -	uap->curregs[R5] &= ~SND_BRK;
> -	pmz_maybe_update_regs(uap);
> -
> -	/* Shut the chip down */
> -	pmz_set_scc_power(uap, 0);
> +	if (!ZS_IS_ASLEEP(uap) && !ZS_IS_CONS(uap))
> +		pmz_set_scc_power(uap, 0);	/* Shut the chip down */
>  
>  	spin_unlock_irqrestore(&port->lock, flags);
>  
> @@ -1352,19 +1357,15 @@ static void pmz_set_termios(struct uart_
>  	spin_lock_irqsave(&port->lock, flags);	
>  
>  	/* Disable IRQs on the port */
> -	uap->curregs[R1] &= ~(EXT_INT_ENAB | TxINT_ENAB | RxINT_MASK);
> -	write_zsreg(uap, R1, uap->curregs[R1]);
> +	pmz_interrupt_control(uap, 0);
>  
>  	/* Setup new port configuration */
>  	__pmz_set_termios(port, termios, old);
>  
>  	/* Re-enable IRQs on the port */
> -	if (ZS_IS_OPEN(uap)) {
> -		uap->curregs[R1] |= INT_ALL_Rx | TxINT_ENAB;
> -		if (!ZS_IS_EXTCLK(uap))
> -			uap->curregs[R1] |= EXT_INT_ENAB;
> -		write_zsreg(uap, R1, uap->curregs[R1]);
> -	}
> +	if (ZS_IS_OPEN(uap))
> +		pmz_interrupt_control(uap, 1);
> +
>  	spin_unlock_irqrestore(&port->lock, flags);
>  }
>  
> @@ -1671,14 +1672,17 @@ static int pmz_suspend(struct macio_dev
>  	spin_lock_irqsave(&uap->port.lock, flags);
>  
>  	if (ZS_IS_OPEN(uap) || ZS_IS_CONS(uap)) {
> -		/* Disable receiver and transmitter.  */
> +		/* Disable interrupt requests for the channel */
> +		pmz_interrupt_control(uap, 0);
> +
> +		/* Disable receiver and transmitter */
>  		uap->curregs[R3] &= ~RxENABLE;
>  		uap->curregs[R5] &= ~TxENABLE;
>  
> -		/* Disable all interrupts and BRK assertion.  */
> -		uap->curregs[R1] &= ~(EXT_INT_ENAB | TxINT_ENAB | RxINT_MASK);
> +		/* Disable break assertion */
>  		uap->curregs[R5] &= ~SND_BRK;
>  		pmz_load_zsregs(uap, uap->curregs);
> +
>  		uap->flags |= PMACZILOG_FLAG_IS_ASLEEP;
>  		mb();
>  	}
> @@ -1738,14 +1742,6 @@ static int pmz_resume(struct macio_dev *
>  	/* Take care of config that may have changed while asleep */
>  	__pmz_set_termios(&uap->port, &uap->termios_cache, NULL);
>  
> -	if (ZS_IS_OPEN(uap)) {
> -		/* Enable interrupts */		
> -		uap->curregs[R1] |= INT_ALL_Rx | TxINT_ENAB;
> -		if (!ZS_IS_EXTCLK(uap))
> -			uap->curregs[R1] |= EXT_INT_ENAB;
> -		write_zsreg(uap, R1, uap->curregs[R1]);
> -	}
> -
>  	spin_unlock_irqrestore(&uap->port.lock, flags);
>  
>  	if (ZS_IS_CONS(uap))
> @@ -1757,6 +1753,12 @@ static int pmz_resume(struct macio_dev *
>  		enable_irq(uap->port.irq);
>  	}
>  
> +	if (ZS_IS_OPEN(uap)) {
> +		spin_lock_irqsave(&uap->port.lock, flags);
> +		pmz_interrupt_control(uap, 1);
> +		spin_unlock_irqrestore(&uap->port.lock, flags);
> +	}
> +
>   bail:
>  	mutex_unlock(&state->port.mutex);
>  	mutex_unlock(&pmz_irq_mutex);