Date:	Wed, 30 Jan 2008 00:47:42 +0100
From:	Jan Kiszka <jan.kiszka@....de>
To:	Jason Wessel <jason.wessel@...driver.com>
CC:	Ingo Molnar <mingo@...e.hu>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	kgdb-bugreport@...ts.sourceforge.net
Subject: [PATCH 5/5] KGDB-8250: more refactorings

Here comes the rest of 8250_kgdb refactorings (the root of the previous
patches - before I realized that there was more to do...). It
basically...

 - reorders functions to avoid prototypes (as far as possible)
 - removes a redundant module parameter help (=>modinfo)
 - fixes some minor style issues

Signed-off-by: Jan Kiszka <jan.kiszka@....de>

---
 drivers/serial/8250_kgdb.c |  452 +++++++++++++++++++++------------------------
 1 file changed, 219 insertions(+), 233 deletions(-)

Index: b/drivers/serial/8250_kgdb.c
===================================================================
--- a/drivers/serial/8250_kgdb.c
+++ b/drivers/serial/8250_kgdb.c
@@ -32,19 +32,27 @@
 
 MODULE_DESCRIPTION("KGDB driver for the 8250");
 MODULE_LICENSE("GPL");
-/* These will conflict with early_param otherwise. */
+
 #ifdef CONFIG_KGDB_8250_MODULE
-static char config[256];
-module_param_string(kgdb8250, config, 256, 0);
-MODULE_PARM_DESC(kgdb8250,
-		 " kgdb8250=<io or mmio>,<address>,<baud rate>,<irq>\n");
-static struct kgdb_io local_kgdb_io_ops;
+/* If built-in, we use early_param instead. */
+static char config_string[256];
+module_param_string(kgdb8250, config_string, sizeof(config_string), 0);
+MODULE_PARM_DESC(kgdb8250, "<io or mmio>,<address>,<baud rate>,<irq> or "
+			   "<port #>,<baud rate>");
+
+static struct kgdb_io kgdb8250_io_ops;
+
 #else  /* !CONFIG_KGDB_8250_MODULE */
 static int params_evaluated;
 #endif /* !CONFIG_KGDB_8250_MODULE */
 
-/* Speed of the UART. */
-static int kgdb8250_baud;
+/* Our internal table of UARTS. */
+#define UART_NR	CONFIG_SERIAL_8250_NR_UARTS
+static struct uart_port kgdb8250_ports[UART_NR];
+static struct uart_port *current_port;
+
+static int kgdb8250_baud;	/* Speed of the UART. */
+static void *kgdb8250_addr;	/* Base of the UART. */
 
 /* Flag for if we need to call request_mem_region */
 static int kgdb8250_needs_request_mem_region;
@@ -53,19 +61,7 @@ static char kgdb8250_buf[GDB_BUF_SIZE];
 static atomic_t kgdb8250_buf_in_cnt;
 static int kgdb8250_buf_out_inx;
 
-/* Our internal table of UARTS. */
-#define UART_NR	CONFIG_SERIAL_8250_NR_UARTS
-static struct uart_port kgdb8250_ports[UART_NR];
-
-static struct uart_port *current_port;
-
-/* Base of the UART. */
-static void *kgdb8250_addr;
-
-/* Forward declarations. */
 static int kgdb8250_uart_init(void);
-static int __init kgdb_init_io(void);
-static int __init kgdb8250_opt(char *str);
 
 /* These are much shorter calls to ioread8/iowrite8 that take into
  * account our shifts, etc. */
@@ -141,8 +137,7 @@ static int kgdb_get_debug_char(void)
  * line we're in charge of.  If this is true, schedule a breakpoint and
  * return.
  */
-static irqreturn_t
-kgdb8250_interrupt(int irq, void *dev_id)
+static irqreturn_t kgdb8250_interrupt(int irq, void *dev_id)
 {
 	if (!(kgdb_ioread(UART_IIR) & UART_IIR_RDI))
 		return IRQ_NONE;
@@ -158,89 +153,6 @@ kgdb8250_interrupt(int irq, void *dev_id
 }
 
 /*
- *  Initializes the UART.
- *  Returns:
- *	0 on success, 1 on failure.
- */
-static int kgdb8250_uart_init(void)
-{
-	unsigned int ier;
-	unsigned int base_baud = current_port->uartclk ?
-		current_port->uartclk / 16 : BASE_BAUD;
-
-	/* test uart existance */
-	if (kgdb_ioread(UART_LSR) == 0xff)
-		return -1;
-
-	/* disable interrupts */
-	kgdb_iowrite(0, UART_IER);
-
-#if defined(CONFIG_ARCH_OMAP1510)
-	/* Workaround to enable 115200 baud on OMAP1510 internal ports */
-	if (cpu_is_omap1510() && is_omap_port((void *)kgdb8250_addr)) {
-		if (kgdb8250_baud == 115200) {
-			base_baud = 1;
-			kgdb8250_baud = 1;
-			kgdb_iowrite(1, UART_OMAP_OSC_12M_SEL);
-		} else
-			kgdb_iowrite(0, UART_OMAP_OSC_12M_SEL);
-	}
-#endif
-	/* set DLAB */
-	kgdb_iowrite(UART_LCR_DLAB, UART_LCR);
-
-	/* set baud */
-	kgdb_iowrite((base_baud / kgdb8250_baud) & 0xff, UART_DLL);
-	kgdb_iowrite((base_baud / kgdb8250_baud) >> 8, UART_DLM);
-
-	/* reset DLAB, set LCR */
-	kgdb_iowrite(UART_LCR_WLEN8, UART_LCR);
-
-	/* set DTR and RTS */
-	kgdb_iowrite(UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS, UART_MCR);
-
-	/* setup fifo */
-	kgdb_iowrite(UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR
-		| UART_FCR_CLEAR_XMIT | UART_FCR_TRIGGER_8,
-		UART_FCR);
-
-	/* clear pending interrupts */
-	kgdb_ioread(UART_IIR);
-	kgdb_ioread(UART_RX);
-	kgdb_ioread(UART_LSR);
-	kgdb_ioread(UART_MSR);
-
-	/* turn on RX interrupt only */
-	kgdb_iowrite(UART_IER_RDI, UART_IER);
-
-	/*
-	 * Borrowed from the main 8250 driver.
-	 * Try writing and reading the UART_IER_UUE bit (b6).
-	 * If it works, this is probably one of the Xscale platform's
-	 * internal UARTs.
-	 * We're going to explicitly set the UUE bit to 0 before
-	 * trying to write and read a 1 just to make sure it's not
-	 * already a 1 and maybe locked there before we even start start.
-	 */
-	ier = kgdb_ioread(UART_IER);
-	kgdb_iowrite(ier & ~UART_IER_UUE, UART_IER);
-	if (!(kgdb_ioread(UART_IER) & UART_IER_UUE)) {
-		/*
-		 * OK it's in a known zero state, try writing and reading
-		 * without disturbing the current state of the other bits.
-		 */
-		kgdb_iowrite(ier | UART_IER_UUE, UART_IER);
-		if (kgdb_ioread(UART_IER) & UART_IER_UUE)
-			/*
-			 * It's an Xscale.
-			 */
-			ier |= UART_IER_UUE | UART_IER_RTOIE;
-	}
-	kgdb_iowrite(ier, UART_IER);
-	return 0;
-}
-
-/*
  * Copy the old serial_state table to our uart_port table if we haven't
  * had values specifically configured in.  We need to make sure this only
  * happens once.
@@ -274,6 +186,91 @@ static void kgdb8250_copy_rs_table(void)
 }
 
 /*
+ * Syntax for this cmdline option is:
+ *  kgdb8250=<io or mmio>,<address>,<baud rate>,<irq> or
+ *  kgdb8250=<port #>,<baud rate>
+ */
+static int __init kgdb8250_opt(char *str)
+{
+	/* Give us the basic table of uarts. */
+	kgdb8250_copy_rs_table();
+
+	/* If we overwrite, we'll fill out and use the first slot. */
+	current_port = &kgdb8250_ports[0];
+
+	if (!strncmp(str, "io", 2)) {
+		current_port->iotype = UPIO_PORT;
+		str += 2;
+	} else if (!strncmp(str, "mmap", 4)) {
+		current_port->iotype = UPIO_MEM;
+		current_port->flags |= UPF_IOREMAP;
+		str += 4;
+	} else if (!strncmp(str, "mmio", 4)) {
+		current_port->iotype = UPIO_MEM;
+		current_port->flags &= ~UPF_IOREMAP;
+		str += 4;
+	} else if (*str >= '0' || *str <= '9') {
+		int line = *str - '0';
+		/* UARTS in the list from 0 -> 9 */
+		if (line >= UART_NR)
+			goto errout;
+		current_port = &kgdb8250_ports[line];
+		if (serial8250_get_port_def(current_port, line) < 0 &&
+		    !current_port->iobase && !current_port->membase)
+			goto errout;
+		str++;
+		if (*str != ',')
+			goto errout;
+		str++;
+		kgdb8250_baud = simple_strtoul(str, &str, 10);
+		if (!kgdb8250_baud)
+			goto errout;
+		if (*str == ',')
+			goto errout;
+		goto finish;
+	} else
+		goto errout;
+
+	if (*str != ',')
+		goto errout;
+	str++;
+
+	if (current_port->iotype == UPIO_PORT)
+		current_port->iobase = simple_strtoul(str, &str, 16);
+	else {
+		if (current_port->flags & UPF_IOREMAP)
+			current_port->mapbase =
+				(unsigned long) simple_strtoul(str, &str, 16);
+		else
+			current_port->membase =
+				(void *) simple_strtoul(str, &str, 16);
+	}
+
+	if (*str != ',')
+		goto errout;
+	str++;
+
+	kgdb8250_baud = simple_strtoul(str, &str, 10);
+	if (!kgdb8250_baud)
+		goto errout;
+
+	if (*str != ',')
+		goto errout;
+	str++;
+
+	current_port->irq = simple_strtoul(str, &str, 10);
+
+finish:
+#ifdef CONFIG_KGDB_8250
+	params_evaluated = 1;
+#endif
+	return 0;
+
+errout:
+	return 1;
+}
+
+/*
  * Hookup our IRQ line now that it is safe to do so, after we grab any
  * memory regions we might need to.  If we haven't been initialized yet,
  * go ahead and copy the old_rs_table in.
@@ -286,7 +283,8 @@ static void __init kgdb8250_late_init(vo
 
 	/* Now reinit the port as the above has disabled things. */
 	kgdb8250_uart_init();
-#endif
+#endif /* CONFIG_SERIAL_8250 || CONFIG_SERIAL_8250_MODULE */
+
 	/* We may need to call request_mem_region() first. */
 	if (kgdb8250_needs_request_mem_region)
 		request_mem_region(current_port->mapbase,
@@ -302,16 +300,8 @@ static __init int kgdb_init_io(void)
 	/* We're either a module and parse a config string, or we have a
 	 * semi-static config. */
 #ifdef CONFIG_KGDB_8250_MODULE
-	if (strlen(config)) {
-		if (kgdb8250_opt(config))
-			return -EINVAL;
-	} else {
-		printk(KERN_ERR "kgdb8250: argument error, usage: "
-		       "kgdb8250=<io or mmio>,<address>,<baud rate>,<irq>\n");
-		printk(KERN_ERR "kgdb8250: alt usage: "
-		       "kgdb8250=<line #>,<baud rate>\n");
+	if (strlen(config_string) || kgdb8250_opt(config_string))
 		return -EINVAL;
-	}
 #else  /* !CONFIG_KGDB_8250_MODULE */
 	if (!params_evaluated) {
 #ifdef CONFIG_KGDB_SIMPLE_SERIAL
@@ -356,49 +346,113 @@ static __init int kgdb_init_io(void)
 		printk(KERN_ERR "kgdb8250: init failed\n");
 		return -EIO;
 	}
+
 #ifdef CONFIG_KGDB_8250_MODULE
-	/* Attach the kgdb irq. When this is built into the kernel, it
+	/*
+	 * Attach the kgdb irq. When this is built into the kernel, it
 	 * is called as a part of late_init sequence.
 	 */
 	kgdb8250_late_init();
-	if (kgdb_register_io_module(&local_kgdb_io_ops))
+	if (kgdb_register_io_module(&kgdb8250_io_ops))
 		return -EINVAL;
 
 	printk(KERN_INFO "kgdb8250: debugging enabled\n");
-#endif				/* CONFIG_KGD_8250_MODULE */
+#endif 	/* CONFIG_KGD_8250_MODULE */
 
 	return 0;
 }
 
-#ifdef CONFIG_KGDB_8250_MODULE
-/* If it is a module the kgdb_io_ops should be a static which
- * is passed to the KGDB I/O initialization
+/*
+ *  Initializes the UART.
+ *  Returns:
+ *	0 on success, 1 on failure.
  */
-static void kgdb8250_pre_exception_handler(void);
-static void kgdb8250_post_exception_handler(void);
+static int kgdb8250_uart_init(void)
+{
+	unsigned int ier;
+	unsigned int base_baud = current_port->uartclk ?
+		current_port->uartclk / 16 : BASE_BAUD;
+ 
+	/* test uart existance */
+	if (kgdb_ioread(UART_LSR) == 0xff)
+		return -1;
+ 
+	/* disable interrupts */
+	kgdb_iowrite(0, UART_IER);
 
-static struct kgdb_io local_kgdb_io_ops = {
-#else				/* ! CONFIG_KGDB_8250_MODULE */
-struct kgdb_io kgdb_io_ops = {
-#endif				/* ! CONFIG_KGD_8250_MODULE */
-	.read_char = kgdb_get_debug_char,
-	.write_char = kgdb_put_debug_char,
-	.init = kgdb_init_io,
-	.late_init = kgdb8250_late_init,
-#ifdef CONFIG_KGDB_8250_MODULE
-	.pre_exception = kgdb8250_pre_exception_handler,
-	.post_exception = kgdb8250_post_exception_handler,
-#endif
-};
+#ifdef CONFIG_ARCH_OMAP1510
+	/* Workaround to enable 115200 baud on OMAP1510 internal ports */
+	if (cpu_is_omap1510() && is_omap_port((void *)kgdb8250_addr)) {
+		if (kgdb8250_baud == 115200) {
+			base_baud = 1;
+			kgdb8250_baud = 1;
+			kgdb_iowrite(1, UART_OMAP_OSC_12M_SEL);
+		} else
+			kgdb_iowrite(0, UART_OMAP_OSC_12M_SEL);
+	}
+#endif /* CONFIG_ARCH_OMAP1510 */
+	/* set DLAB */
+	kgdb_iowrite(UART_LCR_DLAB, UART_LCR);
+
+	/* set baud */
+	kgdb_iowrite((base_baud / kgdb8250_baud) & 0xff, UART_DLL);
+	kgdb_iowrite((base_baud / kgdb8250_baud) >> 8, UART_DLM);
+
+	/* reset DLAB, set LCR */
+	kgdb_iowrite(UART_LCR_WLEN8, UART_LCR);
+
+	/* set DTR and RTS */
+	kgdb_iowrite(UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS, UART_MCR);
+
+	/* setup fifo */
+	kgdb_iowrite(UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR
+		| UART_FCR_CLEAR_XMIT | UART_FCR_TRIGGER_8,
+		UART_FCR);
+
+	/* clear pending interrupts */
+	kgdb_ioread(UART_IIR);
+	kgdb_ioread(UART_RX);
+	kgdb_ioread(UART_LSR);
+	kgdb_ioread(UART_MSR);
+
+	/* turn on RX interrupt only */
+	kgdb_iowrite(UART_IER_RDI, UART_IER);
+
+	/*
+	 * Borrowed from the main 8250 driver.
+	 * Try writing and reading the UART_IER_UUE bit (b6).
+	 * If it works, this is probably one of the Xscale platform's
+	 * internal UARTs.
+	 * We're going to explicitly set the UUE bit to 0 before
+	 * trying to write and read a 1 just to make sure it's not
+	 * already a 1 and maybe locked there before we even start start.
+	 */
+	ier = kgdb_ioread(UART_IER);
+	kgdb_iowrite(ier & ~UART_IER_UUE, UART_IER);
+	if (!(kgdb_ioread(UART_IER) & UART_IER_UUE)) {
+		/*
+		 * OK it's in a known zero state, try writing and reading
+		 * without disturbing the current state of the other bits.
+		 */
+		kgdb_iowrite(ier | UART_IER_UUE, UART_IER);
+		if (kgdb_ioread(UART_IER) & UART_IER_UUE)
+			/*
+			 * It's an Xscale.
+			 */
+			ier |= UART_IER_UUE | UART_IER_RTOIE;
+	}
+	kgdb_iowrite(ier, UART_IER);
+	return 0;
+}
 
 /**
- * 	kgdb8250_add_platform_port - Define a serial port for use with KGDB
- * 	@i: The index of the port being added
- * 	@p: The &struct plat_serial8250_port describing the port
+ * kgdb8250_add_platform_port - Define a serial port for use with KGDB
+ * @i: The index of the port being added
+ * @p: The &struct plat_serial8250_port describing the port
  *
- * 	On platforms where we must register the serial device
- * 	dynamically, this is the best option if a platform normally
- * 	handles uart setup with an array of &struct plat_serial8250_port.
+ * On platforms where we must register the serial device
+ * dynamically, this is the best option if a platform normally
+ * handles uart setup with an array of &struct plat_serial8250_port.
  */
 void __init kgdb8250_add_platform_port(int i, struct plat_serial8250_port *p)
 {
@@ -415,90 +469,6 @@ void __init kgdb8250_add_platform_port(i
 	kgdb8250_ports[i].mapbase = p->mapbase;
 }
 
-/*
- * Syntax for this cmdline option is:
- * kgdb8250=<io or mmio>,<address>,<baud rate>,<irq>"
- */
-static int __init kgdb8250_opt(char *str)
-{
-	/* Give us the basic table of uarts. */
-	kgdb8250_copy_rs_table();
-
-	/* If we overwrite, we'll fill out and use the first slot. */
-	current_port = &kgdb8250_ports[0];
-
-	if (!strncmp(str, "io", 2)) {
-		current_port->iotype = UPIO_PORT;
-		str += 2;
-	} else if (!strncmp(str, "mmap", 4)) {
-		current_port->iotype = UPIO_MEM;
-		current_port->flags |= UPF_IOREMAP;
-		str += 4;
-	} else if (!strncmp(str, "mmio", 4)) {
-		current_port->iotype = UPIO_MEM;
-		current_port->flags &= ~UPF_IOREMAP;
-		str += 4;
-	} else if (*str >= '0' || *str <= '9') {
-		int line = *str - '0';
-		/* UARTS in the list from 0 -> 9 */
-		if (line >= UART_NR)
-			goto errout;
-		current_port = &kgdb8250_ports[line];
-		if (serial8250_get_port_def(current_port, line) < 0 &&
-		    !current_port->iobase && !current_port->membase)
-			goto errout;
-		str++;
-		if (*str != ',')
-			goto errout;
-		str++;
-		kgdb8250_baud = simple_strtoul(str, &str, 10);
-		if (!kgdb8250_baud)
-			goto errout;
-		if (*str == ',')
-			goto errout;
-		goto finish;
-	} else
-		goto errout;
-
-	if (*str != ',')
-		goto errout;
-	str++;
-
-	if (current_port->iotype == UPIO_PORT)
-		current_port->iobase = simple_strtoul(str, &str, 16);
-	else {
-		if (current_port->flags & UPF_IOREMAP)
-			current_port->mapbase =
-				(unsigned long) simple_strtoul(str, &str, 16);
-		else
-			current_port->membase =
-				(void *) simple_strtoul(str, &str, 16);
-	}
-
-	if (*str != ',')
-		goto errout;
-	str++;
-
-	kgdb8250_baud = simple_strtoul(str, &str, 10);
-	if (!kgdb8250_baud)
-		goto errout;
-
-	if (*str != ',')
-		goto errout;
-	str++;
-
-	current_port->irq = simple_strtoul(str, &str, 10);
-
-finish:
-#ifdef CONFIG_KGDB_8250
-	params_evaluated = 1;
-#endif
-	return 0;
-
-errout:
-	return 1;
-}
-
 #ifdef CONFIG_KGDB_8250_MODULE
 static void kgdb8250_pre_exception_handler(void)
 {
@@ -512,19 +482,25 @@ static void kgdb8250_post_exception_hand
 		module_put(THIS_MODULE);
 }
 
+static struct kgdb_io kgdb8250_io_ops = {
+	.read_char = kgdb_get_debug_char,
+	.write_char = kgdb_put_debug_char,
+	.init = kgdb_init_io,
+	.late_init = kgdb8250_late_init,
+	.pre_exception = kgdb8250_pre_exception_handler,
+	.post_exception = kgdb8250_post_exception_handler,
+};
+
 static void cleanup_kgdb8250(void)
 {
-	kgdb_unregister_io_module(&local_kgdb_io_ops);
+	kgdb_unregister_io_module(&kgdb8250_io_ops);
 
-	/* Clean up the irq and memory */
 	free_irq(current_port->irq, current_port);
 
 	if (kgdb8250_needs_request_mem_region)
 		release_mem_region(current_port->mapbase,
 				   8 << current_port->regshift);
-	/* Hook up the serial port back to what it was previously
-	 * hooked up to.
-	 */
+
 #if defined(CONFIG_SERIAL_8250) || defined(CONFIG_SERIAL_8250_MODULE)
 	/* Give the port back to the 8250 driver. */
 	serial8250_register_port(current_port);
@@ -533,6 +509,16 @@ static void cleanup_kgdb8250(void)
 
 module_init(kgdb_init_io);
 module_exit(cleanup_kgdb8250);
-#else				/* ! CONFIG_KGDB_8250_MODULE */
+
+#else  /* !CONFIG_KGDB_8250_MODULE */
+
+/* This is the only I/O driver, thus it defines the global kgdb_io_ops. */
+struct kgdb_io kgdb_io_ops = {
+	.read_char = kgdb_get_debug_char,
+	.write_char = kgdb_put_debug_char,
+	.init = kgdb_init_io,
+	.late_init = kgdb8250_late_init,
+};
+
 early_param("kgdb8250", kgdb8250_opt);
-#endif				/* ! CONFIG_KGDB_8250_MODULE */
+#endif /* !CONFIG_KGDB_8250_MODULE */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists