Use platform device for 8250 registration

My dual G4 PowerMac crashes sometimes when it probes for the (absent) serial ports. Theoretically it's supposed to take a machine check and recover -- but it doesn't always work like that.

This patch removes the defaults from asm/pc_serial.h and uses the code which already existed for ppc64 to register any ports which are found in the device tree. It's slightly updated to work in 32-bit and also on the Pegasos which claims the input frequency is 0Hz.

We could probably remove all the rest of the crap from asm/serial.h and let platforms register their own serial8250 platform devices. And this probably breaks serial ports on PReP, which needs to do likewise.

Quoted 11 lines · David Woodhouse

My dual G4 PowerMac crashes sometimes when it probes for the (absent) serial ports. Theoretically it's supposed to take a machine check and recover -- but it doesn't always work like that.

This patch removes the defaults from asm/pc_serial.h and uses the code which already existed for ppc64 to register any ports which are found in
the device tree. It's slightly updated to work in 32-bit and also on the
Pegasos which claims the input frequency is 0Hz.

Quoted 4 lines

We could probably remove all the rest of the crap from asm/serial.h and
let platforms register their own serial8250 platform devices. And this probably breaks serial ports on PReP, which needs to do likewise.

Hmm, I wondering if we can provide some standard way of handling this for the embedded platforms as well. It would be nice to drop the old style of initialization completely and move to using a platform device always.

Quoted 2 lines

We could probably remove all the rest of the crap from asm/serial.h and let platforms register their own serial8250 platform devices...

Hmm, I wondering if we can provide some standard way of handling this for the embedded platforms as well. It would be nice to drop the old style of initialization completely and move to using a platform device always.

Yes, that's precisely what I meant. Just remove it from the list in serial.h and as Ben says, instantiating a platform device is easy.

static struct plat_serial8250_port my_serial_ports[] = {
    {
        .uartclk = 115200*16,
        .iobase = 0x2f8,
        .irq = 3,
        .flags = ASYNC_BOOT_AUTOCONF;
    },
};

static struct platform_device my_serial_device = {
.name = "serial8250", .dev.platform_data = my_serial_ports,
};

  ... platform_device_register(&my_serial_device);

Quoted 7 lines · Kumar Gala

Quoted 2 lines

We could probably remove all the rest of the crap from asm/serial.h and let platforms register their own serial8250 platform devices...

Hmm, I wondering if we can provide some standard way of handling this for the embedded platforms as well. It would be nice to drop the old style of initialization completely and move to using a platform device always.

Yes, that's precisely what I meant. Just remove it from the list in serial.h and as Ben says, instantiating a platform device is easy.

static struct plat_serial8250_port my_serial_ports[] = {
	{
		.uartclk = 115200*16,
		.iobase = 0x2f8,
		.irq = 3,
		.flags = ASYNC_BOOT_AUTOCONF;
	},
};

So long as you convert arch/ppc/boot/ to this as well, why not (or at least being able to grab the infos from these structs somehow).

Once everyone is on a flat tree, I don't object to killing all of the old-style uart definitions steaming out of <asm-ppc/serial.h>.

Quoted 5 lines

So long as you convert arch/ppc/boot/ to this as well, why not (or at least being able to grab the infos from these structs somehow).

Once everyone is on a flat tree, I don't object to killing all of the old-style uart definitions steaming out of <asm-ppc/serial.h>.

Tom, do have a test system that I can look at converting that you would be willing to test the changes to arch/ppc/boot for me.

Quoted 7 lines · Kumar Gala

Quoted 6 lines

So long as you convert arch/ppc/boot/ to this as well, why not (or at least being able to grab the infos from these structs somehow).

Once everyone is on a flat tree, I don't object to killing all of the old-style uart definitions steaming out of <asm-ppc/serial.h>.

Quoted 3 lines

Tom, do have a test system that I can look at converting that you would be willing to test the changes to arch/ppc/boot for me.

With Matt's patch, you could boot a PReP kernel (no VGA con) :) But if you convert LoPEC, I'll throw the kernel at it.

Quoted 12 lines · Tom Rini

Quoted 9 lines · Kumar Gala

Quoted 8 lines

So long as you convert arch/ppc/boot/ to this as well, why not (or at
least being able to grab the infos from these structs somehow).

Once everyone is on a flat tree, I don't object to killing all of the
old-style uart definitions steaming out of <asm-ppc/serial.h>.

Quoted 6 lines

Quoted 3 lines

Tom, do have a test system that I can look at converting that you would be willing to test the changes to arch/ppc/boot for me.

With Matt's patch, you could boot a PReP kernel (no VGA con) :) But if you convert LoPEC, I'll throw the kernel at it.

Does PReP use bootcode? I thought it was OF based, but what do I know.

Quoted 22 lines · Kumar Gala

Quoted 18 lines · Tom Rini

Quoted 4 lines · Kumar Gala

Quoted 4 lines

So long as you convert arch/ppc/boot/ to this as well, why not (or at
least being able to grab the infos from these structs somehow).

Quoted 8 lines

Quoted 4 lines

Once everyone is on a flat tree, I don't object to killing all of the
old-style uart definitions steaming out of <asm-ppc/serial.h>.

Tom, do have a test system that I can look at converting that you would be willing to test the changes to arch/ppc/boot for me.

With Matt's patch, you could boot a PReP kernel (no VGA con) :) But if you convert LoPEC, I'll throw the kernel at it.

Quoted 1 line

Does PReP use bootcode? I thought it was OF based, but what do I know.

Are you asking on a real machine or on qemu? On real PReP hardwrae you can find both OF and other firmware like PPCBUG...in both cases they dump residual data. On qemu, jmayer wrote the openhackware bios with is supposed to be some minimal OF-like thing...it dumps residual data too.

Quoted 12 lines · Tom Rini

Quoted 9 lines · Kumar Gala

Quoted 8 lines

So long as you convert arch/ppc/boot/ to this as well, why not (or at
least being able to grab the infos from these structs somehow).

Once everyone is on a flat tree, I don't object to killing all of the
old-style uart definitions steaming out of <asm-ppc/serial.h>.

Quoted 6 lines

Quoted 3 lines

Tom, do have a test system that I can look at converting that you would be willing to test the changes to arch/ppc/boot for me.

With Matt's patch, you could boot a PReP kernel (no VGA con) :) But if you convert LoPEC, I'll throw the kernel at it.

Does PReP use bootcode? I thought it was OF based, but what do I know.

This patch removes the defaults from asm/pc_serial.h and uses the code which already existed for ppc64 to register any ports which are found in the device tree.

drivers/serial/8250_of.c renamed from drivers/serial/8250_of.c~ +5 -0

@@ -0,0 +1,199 @@
+#include <linux/kernel.h>
+#include <linux/serial.h>
+#include <linux/serial_8250.h>
+#include <linux/config.h>
+#include <linux/module.h>

drivers/serial/8250_of.c renamed from drivers/serial/8250_of.c~ +45 -0

+
+/*
+ * This function can be used by platforms to "find" legacy serial ports.
+ * It works for "serial" nodes under an "isa" node, and will try to
+ * respect the "ibm,aix-loc" property if any. It works with up to 8
+ * ports.
+ */
+
+#define MAX_LEGACY_SERIAL_PORTS	8
+static int ports_probed = 0;
+
+static struct plat_serial8250_port serial_ports[MAX_LEGACY_SERIAL_PORTS+1];
+static unsigned int old_serial_count;
+
+void __init generic_find_legacy_serial_ports(u64 *physport,
+		unsigned int *default_speed)
+{
+	struct device_node *np;
+	u32 *sizeprop;
+
+	struct isa_reg_property {
+		u32 space;
+		u32 address;
+		u32 size;
+	};
+
+	DBG(" -> generic_find_legacy_serial_port()\n");
+	ports_probed = 1;
+
+	*physport = 0;
+	if (default_speed)
+		*default_speed = 0;
+
+	np = of_find_node_by_path("/");
+	if (!np)
+		return;
+
+	/* First fill our array */
+	for (np = NULL; (np = of_find_node_by_type(np, "serial"));) {
+		struct device_node *isa, *pci;
+		struct isa_reg_property *reg;
+		unsigned long phys_size, addr_size;
+		u64 io_base;
+		u32 *rangesp;
+		u32 *interrupts, *clk, *spd;

drivers/serial/8250_of.c renamed from drivers/serial/8250_of.c~ +38 -0

+		if (reg == NULL)
+			goto next_port;
+		/* We assume the interrupt number isn't translated ... */
+		interrupts = (u32 *)get_property(np, "interrupts", NULL);
+		/* get clock freq. if present */
+		clk = (u32 *)get_property(np, "clock-frequency", NULL);
+		/* get default speed if present */
+		spd = (u32 *)get_property(np, "current-speed", NULL);
+		/* Default to locate at end of array */
+		index = old_serial_count; /* end of the array by default */
+
+		/* If we have a location index, then use it */
+		if (typep && *typep == 'S') {
+			index = simple_strtol(typep+1, NULL, 0) - 1;
+			/* if index is out of range, use end of array instead */
+			if (index >= MAX_LEGACY_SERIAL_PORTS)
+				index = old_serial_count;
+			/* if our index is still out of range, that mean that
+			 * array is full, we could scan for a free slot but that
+			 * make little sense to bother, just skip the port
+			 */
+			if (index >= MAX_LEGACY_SERIAL_PORTS)
+				goto next_port;
+			if (index >= old_serial_count)
+				old_serial_count = index + 1;
+			/* Check if there is a port who already claimed our slot */
+			if (serial_ports[index].iobase != 0) {
+				/* if we still have some room, move it, else override */
+				if (old_serial_count < MAX_LEGACY_SERIAL_PORTS) {
+					DBG("Moved legacy port %d -> %d\n", index,
+					    old_serial_count);
+					serial_ports[old_serial_count++] =
+						serial_ports[index];
+				} else {
+					DBG("Replacing legacy port %d\n", index);
+				}
+			}
+		}

drivers/serial/8250_of.c renamed from drivers/serial/8250_of.c~ +64 -0

+		serial_ports[index].flags = ASYNC_BOOT_AUTOCONF;
+
+		DBG("Added legacy port, index: %d, port: %x, irq: %d, clk: %d\n",
+		    index,
+		    serial_ports[index].iobase,
+		    serial_ports[index].irq,
+		    serial_ports[index].uartclk);
+
+		/* Get phys address of IO reg for port 1 */
+		if (index != 0)
+			goto next_port;
+
+		pci = of_get_parent(isa);
+		if (!pci) {
+			DBG("%s: no pci parent found\n", np->full_name);
+			goto next_port;
+		}
+
+		rangesp = (u32 *)get_property(pci, "ranges", &rlen);
+		if (rangesp == NULL) {
+			of_node_put(pci);
+			goto next_port;
+		}
+		rlen /= 4;
+
+		/* we need the #size-cells of the PCI bridge node itself */
+		phys_size = 1;
+		sizeprop = (u32 *)get_property(pci, "#size-cells", NULL);
+		if (sizeprop != NULL)
+			phys_size = *sizeprop;
+		/* we need the parent #addr-cells */
+		addr_size = prom_n_addr_cells(pci);
+		rentsize = 3 + addr_size + phys_size;
+		io_base = 0;
+		for (;rlen >= rentsize; rlen -= rentsize,rangesp += rentsize) {
+			if (((rangesp[0] >> 24) & 0x3) != 1)
+				continue; /* not IO space */
+			io_base = rangesp[3];
+			if (addr_size == 2)
+				io_base = (io_base << 32) | rangesp[4];
+		}
+		if (io_base != 0) {
+			*physport = io_base + reg->address;
+			if (default_speed && spd)
+				*default_speed = *spd;
+		}
+		of_node_put(pci);
+	next_port:
+		of_node_put(isa);
+	}
+
+	DBG(" <- generic_find_legacy_serial_port()\n");
+}
+
+static struct platform_device serial_device = {
+	.name	= "serial8250",
+	.id	= 0,
+	.dev	= {
+		.platform_data = serial_ports,
+	},
+};
+
+static int __init serial_dev_init(void)
+{

drivers/serial/Kconfig renamed from drivers/serial/Kconfig~ +1 -0

+
 config SERIAL_8250_ACPI
 	bool "8250/16550 device discovery via ACPI namespace"
 	default y if IA64

arch/ppc64/kernel/setup.c renamed from arch/ppc64/kernel/setup.c~ +0 -18

@@ -1147,186 +1147,6 @@ void __init setup_default_decr(void)
 	lpaca->next_jiffy_update_tb = get_tb() + tb_ticks_per_jiffy;
 }
 
-#ifndef CONFIG_PPC_ISERIES
-/*
- * This function can be used by platforms to "find" legacy serial ports.
- * It works for "serial" nodes under an "isa" node, and will try to
- * respect the "ibm,aix-loc" property if any. It works with up to 8
- * ports.
- */
-
-#define MAX_LEGACY_SERIAL_PORTS	8
-static struct plat_serial8250_port serial_ports[MAX_LEGACY_SERIAL_PORTS+1];
-static unsigned int old_serial_count;
-
-void __init generic_find_legacy_serial_ports(u64 *physport,
-		unsigned int *default_speed)
-{
-	struct device_node *np;
-	u32 *sizeprop;
-

arch/ppc64/kernel/setup.c renamed from arch/ppc64/kernel/setup.c~ +0 -152

-
-	DBG(" -> generic_find_legacy_serial_port()\n");
-
-	*physport = 0;
-	if (default_speed)
-		*default_speed = 0;
-
-	np = of_find_node_by_path("/");
-	if (!np)
-		return;
-
-	/* First fill our array */
-	for (np = NULL; (np = of_find_node_by_type(np, "serial"));) {
-		struct device_node *isa, *pci;
-		struct isa_reg_property *reg;
-		unsigned long phys_size, addr_size, io_base;
-		u32 *rangesp;
-		u32 *interrupts, *clk, *spd;
-		char *typep;
-		int index, rlen, rentsize;
-
-		/* Ok, first check if it's under an "isa" parent */
-		isa = of_get_parent(np);
-		if (!isa || strcmp(isa->name, "isa")) {
-			DBG("%s: no isa parent found\n", np->full_name);
-			continue;
-		}
-		
-		/* Now look for an "ibm,aix-loc" property that gives us ordering
-		 * if any...
-		 */
-	 	typep = (char *)get_property(np, "ibm,aix-loc", NULL);
-
-		/* Get the ISA port number */
-		reg = (struct isa_reg_property *)get_property(np, "reg", NULL);	
-		if (reg == NULL)
-			goto next_port;
-		/* We assume the interrupt number isn't translated ... */
-		interrupts = (u32 *)get_property(np, "interrupts", NULL);
-		/* get clock freq. if present */
-		clk = (u32 *)get_property(np, "clock-frequency", NULL);
-		/* get default speed if present */
-		spd = (u32 *)get_property(np, "current-speed", NULL);
-		/* Default to locate at end of array */
-		index = old_serial_count; /* end of the array by default */
-
-		/* If we have a location index, then use it */
-		if (typep && *typep == 'S') {
-			index = simple_strtol(typep+1, NULL, 0) - 1;
-			/* if index is out of range, use end of array instead */
-			if (index >= MAX_LEGACY_SERIAL_PORTS)
-				index = old_serial_count;
-			/* if our index is still out of range, that mean that
-			 * array is full, we could scan for a free slot but that
-			 * make little sense to bother, just skip the port
-			 */
-			if (index >= MAX_LEGACY_SERIAL_PORTS)
-				goto next_port;
-			if (index >= old_serial_count)
-				old_serial_count = index + 1;
-			/* Check if there is a port who already claimed our slot */
-			if (serial_ports[index].iobase != 0) {
-				/* if we still have some room, move it, else override */
-				if (old_serial_count < MAX_LEGACY_SERIAL_PORTS) {
-					DBG("Moved legacy port %d -> %d\n", index,
-					    old_serial_count);
-					serial_ports[old_serial_count++] =
-						serial_ports[index];
-				} else {
-					DBG("Replacing legacy port %d\n", index);
-				}
-			}
-		}
-		if (index >= MAX_LEGACY_SERIAL_PORTS)
-			goto next_port;
-		if (index >= old_serial_count)
-			old_serial_count = index + 1;
-
-		/* Now fill the entry */
-		memset(&serial_ports[index], 0, sizeof(struct plat_serial8250_port));
-		serial_ports[index].uartclk = clk ? *clk : BASE_BAUD * 16;
-		serial_ports[index].iobase = reg->address;
-		serial_ports[index].irq = interrupts ? interrupts[0] : 0;
-		serial_ports[index].flags = ASYNC_BOOT_AUTOCONF;
-
-		DBG("Added legacy port, index: %d, port: %x, irq: %d, clk: %d\n",
-		    index,
-		    serial_ports[index].iobase,
-		    serial_ports[index].irq,
-		    serial_ports[index].uartclk);
-
-		/* Get phys address of IO reg for port 1 */
-		if (index != 0)
-			goto next_port;
-
-		pci = of_get_parent(isa);
-		if (!pci) {
-			DBG("%s: no pci parent found\n", np->full_name);
-			goto next_port;
-		}
-
-		rangesp = (u32 *)get_property(pci, "ranges", &rlen);
-		if (rangesp == NULL) {
-			of_node_put(pci);
-			goto next_port;
-		}
-		rlen /= 4;
-
-		/* we need the #size-cells of the PCI bridge node itself */
-		phys_size = 1;
-		sizeprop = (u32 *)get_property(pci, "#size-cells", NULL);
-		if (sizeprop != NULL)
-			phys_size = *sizeprop;
-		/* we need the parent #addr-cells */
-		addr_size = prom_n_addr_cells(pci);
-		rentsize = 3 + addr_size + phys_size;
-		io_base = 0;
-		for (;rlen >= rentsize; rlen -= rentsize,rangesp += rentsize) {
-			if (((rangesp[0] >> 24) & 0x3) != 1)
-				continue; /* not IO space */
-			io_base = rangesp[3];
-			if (addr_size == 2)
-				io_base = (io_base << 32) | rangesp[4];
-		}
-		if (io_base != 0) {
-			*physport = io_base + reg->address;
-			if (default_speed && spd)
-				*default_speed = *spd;
-		}
-		of_node_put(pci);
-	next_port:
-		of_node_put(isa);
-	}
-
-	DBG(" <- generic_find_legacy_serial_port()\n");
-}
-
-static struct platform_device serial_device = {
-	.name	= "serial8250",
-	.id	= 0,
-	.dev	= {
-		.platform_data = serial_ports,
-	},
-};
-
-static int __init serial_dev_init(void)
-{
-	return platform_device_register(&serial_device);
-}
-arch_initcall(serial_dev_init);
-
-#endif /* CONFIG_PPC_ISERIES */
 
 int check_legacy_ioport(unsigned long base_port)
 {

include/asm-ppc/pc_serial.h renamed from include/asm-ppc/pc_serial.h~ +0 -15

@@ -26,18 +26,3 @@
 #define RS_TABLE_SIZE  4
 #endif
 
-/* Standard COM flags (except for COM4, because of the 8514 problem) */
-#ifdef CONFIG_SERIAL_DETECT_IRQ
-#define STD_COM_FLAGS (ASYNC_BOOT_AUTOCONF | ASYNC_SKIP_TEST | ASYNC_AUTO_IRQ)
-#define STD_COM4_FLAGS (ASYNC_BOOT_AUTOCONF | ASYNC_AUTO_IRQ)
-#else
-#define STD_COM_FLAGS (ASYNC_BOOT_AUTOCONF | ASYNC_SKIP_TEST)
-#define STD_COM4_FLAGS ASYNC_BOOT_AUTOCONF
-#endif
-
-#define SERIAL_PORT_DFNS			\
-	/* UART CLK   PORT IRQ     FLAGS        */			\
-	{ 0, BASE_BAUD, 0x3F8, 4, STD_COM_FLAGS },	/* ttyS0 */	\
-	{ 0, BASE_BAUD, 0x2F8, 3, STD_COM_FLAGS },	/* ttyS1 */	\
-	{ 0, BASE_BAUD, 0x3E8, 4, STD_COM_FLAGS },	/* ttyS2 */	\
-	{ 0, BASE_BAUD, 0x2E8, 3, STD_COM4_FLAGS },	/* ttyS3 */

Doesn't PPC64 have NO_IRQ ?

Quoted 1 line · Russell King

Doesn't PPC64 have NO_IRQ ?

PPC64 does but I find no definition of is_real_interrupt() other than the bogus one in 8250.c which checks for zero. PPC32 does not -- let's clean that one up separately.

My dual G4 PowerMac crashes sometimes when it probes for the (absent) serial ports. Theoretically it's supposed to take a machine check and recover -- but it doesn't always work like that.

David's 8250 cleanup patch made me wondering when are machine checks suppose to recoverable? General class of conditions is what I'm looking for here.

Is David's case due to some PCI master abort or something else?

Quoted 5 lines · Kumar Gala

David's 8250 cleanup patch made me wondering when are machine checks suppose to recoverable? General class of conditions is what I'm looking for here.

Is David's case due to some PCI master abort or something else?

Might be some issue on SMP machines...

Quoted 9 lines · Benjamin Herrenschmidt

Quoted 7 lines · Kumar Gala

David's 8250 cleanup patch made me wondering when are machine checks

suppose to recoverable? General class of conditions is what I'm looking for here.

Quoted 5 lines

Quoted 2 lines

Is David's case due to some PCI master abort or something else?

Might be some issue on SMP machines...

Yeah, that'll probably be the reason it turns out _not_ to be recoverable despite our expectations. But that wasn't Kumar's question.

Quoted 17 lines · David Woodhouse

Quoted 14 lines · Benjamin Herrenschmidt

Quoted 9 lines · Kumar Gala

David's 8250 cleanup patch made me wondering when are machine checks

suppose to recoverable? General class of conditions is what I'm looking for here.

Is David's case due to some PCI master abort or something else?

Might be some issue on SMP machines...

Quoted 3 lines

Yeah, that'll probably be the reason it turns out _not_ to be recoverable despite our expectations. But that wasn't Kumar's question.

David's right, that wasn't my question :) I was asking more about what cases do we actually recover and that is considered correct behavior.

"My dual G4 PowerMac crashes sometimes when it probes for the (absent) serial ports. Theoretically it's supposed to take a machine check and recover -- but it doesn't always work like that."

Maybe you just need the proper entry in the exception table for IO inb/outb?

Is there a stacktrace, David?

Quoted 9 lines · Marcelo Tosatti

"My dual G4 PowerMac crashes sometimes when it probes for the (absent) serial ports. Theoretically it's supposed to take a machine check and recover -- but it doesn't always work like that."

Maybe you just need the proper entry in the exception table for IO inb/outb?

Is there a stacktrace, David?

No, just a silent death when probing for one of the serial ports; usually not the first. It doesn't seem to even make it into the machine check handler. Adding printks into serial_inb and serial_outb makes it much less likely to happen -- could be something bizarre about alignment of the instructions which cause the machine check, or time elapsed from one machine check to the next.

If the machine is locking hard it could be check stopping due to getting a second machine check.