Patchwork [01/16] pmac_zilog: fix unexpected irq

login
register
mail settings
Submitter Finn Thain
Date Nov. 24, 2011, 2:34 p.m.
Message ID <alpine.LNX.2.00.1111250133321.6314@nippy.intranet>
Download mbox | patch
Permalink /patch/127531/
State Changes Requested
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Finn Thain - Nov. 24, 2011, 2:34 p.m.
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.

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 opened and SCC chip interrupts 
become enabled, the machine locks up with "unexpected interrupt" because 
request_irq() hasn't happened yet.

Fix this by setting the SCC master interrupt enable bit only after the 
handler is installed. This is achieved by extracting that operation out of 
__pmz_startup() and placing it into a seperate routine.

A similar problem arises when the irq is freed. Fix this by resetting the 
chip first (on m68k mac).

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

---

This patch has been tested on a variety of m68k Macs but no PowerMacs.

I am re-sending this patch Cc linux-serial. It still needs a suitable ack 
so that Geert can push it through his tree.
Alan Cox - Nov. 24, 2011, 2:56 p.m.
On Fri, 25 Nov 2011 01:34:58 +1100 (EST)
Finn Thain <fthain@telegraphics.com.au> 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.
> 
> 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 opened and SCC chip interrupts 
> become enabled, the machine locks up with "unexpected interrupt" because 
> request_irq() hasn't happened yet.
> 
> Fix this by setting the SCC master interrupt enable bit only after the 
> handler is installed. This is achieved by extracting that operation out of 
> __pmz_startup() and placing it into a seperate routine.
> 
> A similar problem arises when the irq is freed. Fix this by resetting the 
> chip first (on m68k mac).
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
> ---
> 
> This patch has been tested on a variety of m68k Macs but no PowerMacs.
> 
> I am re-sending this patch Cc linux-serial. It still needs a suitable ack 
> so that Geert can push it through his tree.

Given the change should work for all hardware do we really need the
ifdefs. Far better I would have thought to just change it so we don't
have to maintain what is effectively two versions of the code between now
and 2038.

So no ack from me yet - I'd like to understand the ifdef decision first.
Otherwise it looks sensible.

Alan
David Laight - Nov. 24, 2011, 3:28 p.m.
> 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.

Wouldn't this also happen if the interrupt were shared?
Hopefully nothing vaguely modern uses the borked Zilog 8530 SCC
(which I presume is the part in question - brings back
too many nightmares....)

	David
Benjamin Herrenschmidt - Nov. 24, 2011, 8:41 p.m.
On Thu, 2011-11-24 at 14:56 +0000, Alan Cox wrote:
> > This patch has been tested on a variety of m68k Macs but no
> PowerMacs.
> > 
> > I am re-sending this patch Cc linux-serial. It still needs a
> suitable ack 
> > so that Geert can push it through his tree.
> 
> Given the change should work for all hardware do we really need the
> ifdefs. Far better I would have thought to just change it so we don't
> have to maintain what is effectively two versions of the code between
> now
> and 2038.
> 
> So no ack from me yet - I'd like to understand the ifdef decision
> first.
> Otherwise it looks sensible.

Yes, agreed. Sorry, that one was one my to-do list for a while, I meant
to look into more details and test on a ppc or two here see if it breaks
anything, and never got to do it.

I'll try to give it a go before hell freezes over.

Cheers,
Ben.
Benjamin Herrenschmidt - Nov. 24, 2011, 8:43 p.m.
On Thu, 2011-11-24 at 15:28 +0000, David Laight 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.
> 
> Wouldn't this also happen if the interrupt were shared?
> Hopefully nothing vaguely modern uses the borked Zilog 8530 SCC
> (which I presume is the part in question - brings back
> too many nightmares....)

Yup. Afaik, the most recent you can find with that are PowerMacs which
used it for their internal modem (even my G5 has one wired to the
internal slot afaik), tho none of those had shared interrupts.

Cheers,
Ben.
Finn Thain - Nov. 25, 2011, 3:15 a.m.
On Thu, 24 Nov 2011, Alan Cox wrote:

> Given the change should work for all hardware do we really need the 
> ifdefs. Far better I would have thought to just change it so we don't 
> have to maintain what is effectively two versions of the code between 
> now and 2038.

I agree.

> 
> So no ack from me yet - I'd like to understand the ifdef decision first.

Removing ifdefs makes the changes more invasive and the suspend/resume 
code then has to be addressed, which I've avoided.

The suspend/resume code path can't be tested on m68k macs and the common 
code paths I can't easily test on a powermac.

This patch should not be needed because the chip reset shouldn't leave the 
tx and rx interrupts enabled. Those interrupts are explicitly enabled only 
after request_irq(), so patching the master interrupt enable behaviour 
should be redundant. But that's not the case in practice.

The chip reset code is already messy. I was inclined towards ifdefs and 
reluctant to share more code after practical experience suggested possible 
differences in the SCC/ESCC devices.

I guess I was hoping that the powermac maintainers might prefer ifdefs to 
increased risk of destabilising the driver on powermacs...

But a more invasive patch would make for better code. I will see if I can 
borrow a suitable PCI PowerMac.

Finn

> Otherwise it looks sensible.
> 
> Alan
Benjamin Herrenschmidt - Nov. 28, 2011, 12:30 a.m.
> Removing ifdefs makes the changes more invasive and the suspend/resume 
> code then has to be addressed, which I've avoided.
> 
> The suspend/resume code path can't be tested on m68k macs and the common 
> code paths I can't easily test on a powermac.
> 
> This patch should not be needed because the chip reset shouldn't leave the 
> tx and rx interrupts enabled. Those interrupts are explicitly enabled only 
> after request_irq(), so patching the master interrupt enable behaviour 
> should be redundant. But that's not the case in practice.
> 
> The chip reset code is already messy. I was inclined towards ifdefs and 
> reluctant to share more code after practical experience suggested possible 
> differences in the SCC/ESCC devices.
> 
> I guess I was hoping that the powermac maintainers might prefer ifdefs to 
> increased risk of destabilising the driver on powermacs...
> 
> But a more invasive patch would make for better code. I will see if I can 
> borrow a suitable PCI PowerMac.

Please do the more invasive patch, I'll beat it up on powermacs.

Cheers,
Ben.

Patch

Index: linux-m68k/drivers/tty/serial/pmac_zilog.c
===================================================================
--- linux-m68k.orig/drivers/tty/serial/pmac_zilog.c	2011-10-22 23:02:22.000000000 +1100
+++ linux-m68k/drivers/tty/serial/pmac_zilog.c	2011-10-22 23:02:38.000000000 +1100
@@ -910,8 +910,8 @@  static int __pmz_startup(struct uart_pma
 	/* Clear handshaking, enable BREAK interrupts */
 	uap->curregs[R15] = BRKIE;
 
-	/* Master interrupt enable */
-	uap->curregs[R9] |= NV | MIE;
+	/* No vector */
+	uap->curregs[R9] |= NV;
 
 	pmz_load_zsregs(uap, uap->curregs);
 
@@ -925,6 +925,17 @@  static int __pmz_startup(struct uart_pma
 	return pwr_delay;
 }
 
+static void pmz_master_int_control(struct uart_pmac_port *uap, int enable)
+{
+	if (enable) {
+		uap->curregs[R9] |= MIE; /* Master interrupt enable */
+		write_zsreg(uap, R9, uap->curregs[R9]);
+	} else {
+		uap->curregs[R9] &= ~MIE;
+		write_zsreg(uap, 9, FHWRES);
+	}
+}
+
 static void pmz_irda_reset(struct uart_pmac_port *uap)
 {
 	uap->curregs[R5] |= DTR;
@@ -976,6 +987,19 @@  static int pmz_startup(struct uart_port
 		return -ENXIO;
 	}
 
+	/*
+	 * Most 68k Mac models cannot mask the SCC IRQ so they must enable
+	 * interrupts after the handler is installed and not before.
+	 */
+#ifndef CONFIG_MAC
+	if (!ZS_IS_CONS(uap))
+#endif
+	{
+		spin_lock_irqsave(&port->lock, flags);
+		pmz_master_int_control(uap, 1);
+		spin_unlock_irqrestore(&port->lock, flags);
+	}
+
 	mutex_unlock(&pmz_irq_mutex);
 
 	/* Right now, we deal with delay by blocking here, I'll be
@@ -1015,6 +1039,11 @@  static void pmz_shutdown(struct uart_por
 
 	mutex_lock(&pmz_irq_mutex);
 
+#ifdef CONFIG_MAC
+	if (!ZS_IS_OPEN(uap->mate))
+		pmz_master_int_control(uap, 0);
+#endif
+
 	/* Release interrupt handler */
 	free_irq(uap->port.irq, uap);
 
@@ -1734,6 +1763,7 @@  static int pmz_resume(struct macio_dev *
 		goto bail;
 	}
 	pwr_delay = __pmz_startup(uap);
+	pmz_master_int_control(uap, 1);
 
 	/* Take care of config that may have changed while asleep */
 	__pmz_set_termios(&uap->port, &uap->termios_cache, NULL);
@@ -2178,6 +2208,9 @@  static int __init pmz_console_setup(stru
 	 * Enable the hardware
 	 */
 	pwr_delay = __pmz_startup(uap);
+#ifndef CONFIG_MAC
+	pmz_master_int_control(uap, 1);
+#endif
 	if (pwr_delay)
 		mdelay(pwr_delay);