[PATCH 1/3] tty: serial: Use hub6_serial_X when CONFIG_SERIAL_8250_HUB6 is defined

[PATCH 1/3] tty: serial: Use hub6_serial_X when CONFIG_SERIAL_8250_HUB6 is defined

[Nobuhiro Iwamatsu]

hub6_serial_X comes to be always usable.
This revises it to be usable when CONFIG_SERIAL_8250_HUB6 is defined.

Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
---
 drivers/tty/serial/8250.c        |   13 ++++++++++++-
 drivers/tty/serial/serial_core.c |    4 ++++
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 3975df6..762a253 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -387,6 +387,7 @@ static inline int map_8250_out_reg(struct uart_port *p, int offset)
 
 #endif
 
+#if CONFIG_SERIAL_8250_HUB6
 static unsigned int hub6_serial_in(struct uart_port *p, int offset)
 {
 	offset = map_8250_in_reg(p, offset) << p->regshift;
@@ -400,6 +401,7 @@ static void hub6_serial_out(struct uart_port *p, int offset, int value)
 	outb(p->hub6 - 1 + offset, p->iobase);
 	outb(value, p->iobase + 1);
 }
+#endif
 
 static unsigned int mem_serial_in(struct uart_port *p, int offset)
 {
@@ -508,10 +510,12 @@ static void set_io_from_upio(struct uart_port *p)
 	struct uart_8250_port *up =
 		container_of(p, struct uart_8250_port, port);
 	switch (p->iotype) {
+#if CONFIG_SERIAL_8250_HUB6
 	case UPIO_HUB6:
 		p->serial_in = hub6_serial_in;
 		p->serial_out = hub6_serial_out;
 		break;
+#endif
 
 	case UPIO_MEM:
 		p->serial_in = mem_serial_in;
@@ -2538,8 +2542,9 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
 			}
 		}
 		break;
-
+#if CONFIG_SERIAL_8250_HUB6
 	case UPIO_HUB6:
+#endif
 	case UPIO_PORT:
 		if (!request_region(up->port.iobase, size, "serial"))
 			ret = -EBUSY;
@@ -2570,7 +2575,9 @@ static void serial8250_release_std_resource(struct uart_8250_port *up)
 		release_mem_region(up->port.mapbase, size);
 		break;
 
+#if CONFIG_SERIAL_8250_HUB6
 	case UPIO_HUB6:
+#endif
 	case UPIO_PORT:
 		release_region(up->port.iobase, size);
 		break;
@@ -2584,7 +2591,9 @@ static int serial8250_request_rsa_resource(struct uart_8250_port *up)
 	int ret = -EINVAL;
 
 	switch (up->port.iotype) {
+#if CONFIG_SERIAL_8250_HUB6
 	case UPIO_HUB6:
+#endif
 	case UPIO_PORT:
 		start += up->port.iobase;
 		if (request_region(start, size, "serial-rsa"))
@@ -2603,7 +2612,9 @@ static void serial8250_release_rsa_resource(struct uart_8250_port *up)
 	unsigned int size = 8 << up->port.regshift;
 
 	switch (up->port.iotype) {
+#if CONFIG_SERIAL_8250_HUB6
 	case UPIO_HUB6:
+#endif
 	case UPIO_PORT:
 		release_region(up->port.iobase + offset, size);
 		break;
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 460a72d..c533524 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2129,10 +2129,12 @@ uart_report_port(struct uart_driver *drv, struct uart_port *port)
 	case UPIO_PORT:
 		snprintf(address, sizeof(address), "I/O 0x%lx", port->iobase);
 		break;
+#if CONFIG_SERIAL_8250_HUB6
 	case UPIO_HUB6:
 		snprintf(address, sizeof(address),
 			 "I/O 0x%lx offset 0x%x", port->iobase, port->hub6);
 		break;
+#endif
 	case UPIO_MEM:
 	case UPIO_MEM32:
 	case UPIO_AU:
@@ -2551,9 +2553,11 @@ int uart_match_port(struct uart_port *port1, struct uart_port *port2)
 	switch (port1->iotype) {
 	case UPIO_PORT:
 		return (port1->iobase == port2->iobase);
+#if CONFIG_SERIAL_8250_HUB6
 	case UPIO_HUB6:
 		return (port1->iobase == port2->iobase) &&
 		       (port1->hub6   == port2->hub6);
+#endif
 	case UPIO_MEM:
 	case UPIO_MEM32:
 	case UPIO_AU:
-- 
1.7.2.3

[PATCH 2/3] tty: serial: Fix build on architecture that does not have ioport

[Nobuhiro Iwamatsu]

Some CPU's do not have ioport. Therefore, these do not have inX/outX.

This adds CONFIG_SERIAL_8250_IOPORT. When this is enable, 8250 driver
provides ioports access. And this is not enable with the CPU without
ioports.

Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
---
 drivers/tty/serial/8250.c        |   56 ++++++++++++++++---------------------
 drivers/tty/serial/8250_early.c  |    4 +++
 drivers/tty/serial/Kconfig       |   10 +++++++
 drivers/tty/serial/serial_core.c |    4 +++
 4 files changed, 42 insertions(+), 32 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 762a253..837bb39 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -387,7 +387,7 @@ static inline int map_8250_out_reg(struct uart_port *p, int offset)
 
 #endif
 
-#if CONFIG_SERIAL_8250_HUB6
+#ifdef CONFIG_SERIAL_8250_HUB6
 static unsigned int hub6_serial_in(struct uart_port *p, int offset)
 {
 	offset = map_8250_in_reg(p, offset) << p->regshift;
@@ -492,7 +492,7 @@ static void dwapb32_serial_out(struct uart_port *p, int offset, int value)
 	writel(value, p->membase + offset);
 	dwapb_check_clear_ier(p, save_offset);
 }
-
+#ifdef CONFIG_SERIAL_8250_IOPORT
 static unsigned int io_serial_in(struct uart_port *p, int offset)
 {
 	offset = map_8250_in_reg(p, offset) << p->regshift;
@@ -504,13 +504,14 @@ static void io_serial_out(struct uart_port *p, int offset, int value)
 	offset = map_8250_out_reg(p, offset) << p->regshift;
 	outb(value, p->iobase + offset);
 }
+#endif
 
 static void set_io_from_upio(struct uart_port *p)
 {
 	struct uart_8250_port *up =
 		container_of(p, struct uart_8250_port, port);
 	switch (p->iotype) {
-#if CONFIG_SERIAL_8250_HUB6
+#ifdef CONFIG_SERIAL_8250_HUB6
 	case UPIO_HUB6:
 		p->serial_in = hub6_serial_in;
 		p->serial_out = hub6_serial_out;
@@ -549,8 +550,10 @@ static void set_io_from_upio(struct uart_port *p)
 		break;
 
 	default:
+#ifdef CONFIG_SERIAL_8250_IOPORT
 		p->serial_in = io_serial_in;
 		p->serial_out = io_serial_out;
+#endif
 		break;
 	}
 	/* Remember loaded iotype */
@@ -2542,12 +2545,11 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
 			}
 		}
 		break;
-#if CONFIG_SERIAL_8250_HUB6
-	case UPIO_HUB6:
-#endif
-	case UPIO_PORT:
+	default: /* UPIO_HUB6 or UPIO_PORT */
+#if defined(CONFIG_SERIAL_8250_HUB6) || defined(CONFIG_SERIAL_8250_IOPORT)
 		if (!request_region(up->port.iobase, size, "serial"))
 			ret = -EBUSY;
+#endif
 		break;
 	}
 	return ret;
@@ -2575,50 +2577,40 @@ static void serial8250_release_std_resource(struct uart_8250_port *up)
 		release_mem_region(up->port.mapbase, size);
 		break;
 
-#if CONFIG_SERIAL_8250_HUB6
-	case UPIO_HUB6:
-#endif
-	case UPIO_PORT:
+	default: /* UPIO_HUB6 or UPIO_PORT */
+#if defined(CONFIG_SERIAL_8250_HUB6) || defined(CONFIG_SERIAL_8250_IOPORT)
 		release_region(up->port.iobase, size);
+#endif
 		break;
 	}
 }
 
 static int serial8250_request_rsa_resource(struct uart_8250_port *up)
 {
+	int ret = -EINVAL;
+
+#if defined(CONFIG_SERIAL_8250_HUB6) || defined(CONFIG_SERIAL_8250_IOPORT)
 	unsigned long start = UART_RSA_BASE << up->port.regshift;
 	unsigned int size = 8 << up->port.regshift;
-	int ret = -EINVAL;
 
-	switch (up->port.iotype) {
-#if CONFIG_SERIAL_8250_HUB6
-	case UPIO_HUB6:
+	/* up->port.iotype are UPIO_HUB6 or UPIO_PORT */
+	start += up->port.iobase;
+	if (request_region(start, size, "serial-rsa"))
+		ret = 0;
+	else
+		ret = -EBUSY;
 #endif
-	case UPIO_PORT:
-		start += up->port.iobase;
-		if (request_region(start, size, "serial-rsa"))
-			ret = 0;
-		else
-			ret = -EBUSY;
-		break;
-	}
-
 	return ret;
 }
 
 static void serial8250_release_rsa_resource(struct uart_8250_port *up)
 {
+#if defined(CONFIG_SERIAL_8250_HUB6) || defined(CONFIG_SERIAL_8250_IOPORT)
 	unsigned long offset = UART_RSA_BASE << up->port.regshift;
 	unsigned int size = 8 << up->port.regshift;
-
-	switch (up->port.iotype) {
-#if CONFIG_SERIAL_8250_HUB6
-	case UPIO_HUB6:
+	/* up->port.iotype are UPIO_HUB6 or UPIO_PORT */
+	release_region(up->port.iobase + offset, size);
 #endif
-	case UPIO_PORT:
-		release_region(up->port.iobase + offset, size);
-		break;
-	}
 }
 
 static void serial8250_release_port(struct uart_port *port)
diff --git a/drivers/tty/serial/8250_early.c b/drivers/tty/serial/8250_early.c
index eaafb98..6eae7a3 100644
--- a/drivers/tty/serial/8250_early.c
+++ b/drivers/tty/serial/8250_early.c
@@ -55,8 +55,10 @@ static unsigned int __init serial_in(struct uart_port *port, int offset)
 		return readb(port->membase + offset);
 	case UPIO_MEM32:
 		return readl(port->membase + (offset << 2));
+#if CONFIG_SERIAL_8250_IOPORT
 	case UPIO_PORT:
 		return inb(port->iobase + offset);
+#endif
 	default:
 		return 0;
 	}
@@ -71,9 +73,11 @@ static void __init serial_out(struct uart_port *port, int offset, int value)
 	case UPIO_MEM32:
 		writel(value, port->membase + (offset << 2));
 		break;
+#if CONFIG_SERIAL_8250_IOPORT
 	case UPIO_PORT:
 		outb(value, port->iobase + offset);
 		break;
+#endif
 	}
 }
 
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 2b83346..6fd5825 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -116,6 +116,16 @@ config SERIAL_8250_CS
 
 	  If unsure, say N.
 
+config CONFIG_SERIAL_8250_IOPORT
+	bool "8250/16550 serial IOports access support"
+	depends on SERIAL_8250 && NO_IOPORT != y
+	default SERIAL_8250
+	help
+	  Say Y here to enable support for serial ioport access.
+	  If there is not ioport, cannot be enable this.
+
+	  If unsure, say N.
+
 config SERIAL_8250_NR_UARTS
 	int "Maximum number of 8250/16550 serial ports"
 	depends on SERIAL_8250
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c533524..ad5b215 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2126,9 +2126,11 @@ uart_report_port(struct uart_driver *drv, struct uart_port *port)
 	char address[64];
 
 	switch (port->iotype) {
+#if CONFIG_SERIAL_8250_IOPORT
 	case UPIO_PORT:
 		snprintf(address, sizeof(address), "I/O 0x%lx", port->iobase);
 		break;
+#endif
 #if CONFIG_SERIAL_8250_HUB6
 	case UPIO_HUB6:
 		snprintf(address, sizeof(address),
@@ -2551,8 +2553,10 @@ int uart_match_port(struct uart_port *port1, struct uart_port *port2)
 		return 0;
 
 	switch (port1->iotype) {
+#if CONFIG_SERIAL_8250_IOPORT
 	case UPIO_PORT:
 		return (port1->iobase == port2->iobase);
+#endif
 #if CONFIG_SERIAL_8250_HUB6
 	case UPIO_HUB6:
 		return (port1->iobase == port2->iobase) &&
-- 
1.7.2.3

Re: [PATCH 2/3] tty: serial: Fix build on architecture that does not have ioport

[Alan Cox]

On Fri, 11 Mar 2011 21:58:11 +0900
Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> wrote:

> Some CPU's do not have ioport. Therefore, these do not have inX/outX.

The results of doing that are usually horrible and add a lot of
configuration and ifdefs.

> This adds CONFIG_SERIAL_8250_IOPORT. When this is enable, 8250 driver
> provides ioports access. And this is not enable with the CPU without
> ioports.

NAK this - all the ifdefs make it hard to maintain. If your platform does
not support inb or outb then it is easier and needs no device
modifications if you just have an architecture file which is something
like

u8 inb(....)
{
	WARN_ON(1);
	return 0xFF;
}

etc

You will then get a backtrace if the methods are called rather than
having to hack all the drivers. In addition if your platform ever has a
PCI bridge attached to it you will now be able to replace those methods
and provide PCI I/O space access, as you will need for many PCI devices.

Re: [PATCH 2/3] tty: serial: Fix build on architecture that does not have ioport

[Arnd Bergmann]

On Friday 11 March 2011, Alan Cox wrote:
> > Some CPU's do not have ioport. Therefore, these do not have inX/outX.
> 
> The results of doing that are usually horrible and add a lot of
> configuration and ifdefs.

I guess a different implementation of this patch could convert the
driver to use ioport_map and ioread/iowrite to get rid of the
#ifdef. This might even make the driver simpler because it
does not have to special-case mmio vs. pio accesses.

Note that the 8250 driver is the basically the only driver that
uses inb/outb that can be built without setting CONFIG_ISA
or CONFIG_PCI.
 
> You will then get a backtrace if the methods are called rather than
> having to hack all the drivers. In addition if your platform ever has a
> PCI bridge attached to it you will now be able to replace those methods
> and provide PCI I/O space access, as you will need for many PCI devices.

That does not seem easier than writing new I/O space functions and
removing CONFIG_NO_IOPORT.

	Arnd

[PATCH 3/3] tty: serial: Check UPF_FOURPORT, when CONFIG_SERIAL_8250_FOURPORT is defined

[Nobuhiro Iwamatsu]

When CONFIG_SERIAL_8250_FOURPORT is defined, driver uses UPF_FOURPORT in port.flags.
Though CONFIG_SERIAL_8250_FOURPORT is not defined, there is a case to check UPF_FOURPORT.
When CONFIG_SERIAL_8250_FOURPORT is defined only, this checks UPF_FOURPORT.

Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/tty/serial/8250.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 837bb39..6e16ce2 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -1297,17 +1297,20 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
 static void autoconfig_irq(struct uart_8250_port *up)
 {
 	unsigned char save_mcr, save_ier;
-	unsigned char save_ICP = 0;
-	unsigned int ICP = 0;
 	unsigned long irqs;
 	int irq;
 
+#ifdef CONFIG_SERIAL_8250_FOURPORT
+	unsigned char save_ICP = 0;
+	unsigned int ICP = 0;
+
 	if (up->port.flags & UPF_FOURPORT) {
 		ICP = (up->port.iobase & 0xfe0) | 0x1f;
 		save_ICP = inb_p(ICP);
 		outb_p(0x80, ICP);
 		(void) inb_p(ICP);
 	}
+#endif /* CONFIG_SERIAL_8250_FOURPORT */
 
 	/* forget possible initially masked and pending IRQ */
 	probe_irq_off(probe_irq_on());
@@ -1337,8 +1340,10 @@ static void autoconfig_irq(struct uart_8250_port *up)
 	serial_outp(up, UART_MCR, save_mcr);
 	serial_outp(up, UART_IER, save_ier);
 
+#ifdef CONFIG_SERIAL_8250_FOURPORT
 	if (up->port.flags & UPF_FOURPORT)
 		outb_p(save_ICP, ICP);
+#endif
 
 	up->port.irq = (irq > 0) ? irq : 0;
 }
@@ -2199,6 +2204,7 @@ dont_test_tx_en:
 	up->ier = UART_IER_RLSI | UART_IER_RDI;
 	serial_outp(up, UART_IER, up->ier);
 
+#ifdef CONFIG_SERIAL_8250_FOURPORT
 	if (up->port.flags & UPF_FOURPORT) {
 		unsigned int icp;
 		/*
@@ -2208,6 +2214,7 @@ dont_test_tx_en:
 		outb_p(0x80, icp);
 		(void) inb_p(icp);
 	}
+#endif
 
 	return 0;
 }
@@ -2225,11 +2232,13 @@ static void serial8250_shutdown(struct uart_port *port)
 	serial_outp(up, UART_IER, 0);
 
 	spin_lock_irqsave(&up->port.lock, flags);
+#ifdef CONFIG_SERIAL_8250_FOURPORT
 	if (up->port.flags & UPF_FOURPORT) {
 		/* reset interrupts on the AST Fourport board */
 		inb((up->port.iobase & 0xfe0) | 0x1f);
 		up->port.mctrl |= TIOCM_OUT1;
 	} else
+#endif
 		up->port.mctrl &= ~TIOCM_OUT2;
 
 	serial8250_set_mctrl(&up->port, up->port.mctrl);
-- 
1.7.2.3

Re: [PATCH 3/3] tty: serial: Check UPF_FOURPORT, when CONFIG_SERIAL_8250_FOURPORT is defined

[Alan Cox]

On Fri, 11 Mar 2011 21:58:12 +0900
Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> wrote:

> When CONFIG_SERIAL_8250_FOURPORT is defined, driver uses UPF_FOURPORT in port.flags.
> Though CONFIG_SERIAL_8250_FOURPORT is not defined, there is a case to check UPF_FOURPORT.
> When CONFIG_SERIAL_8250_FOURPORT is defined only, this checks UPF_FOURPORT.

Again all this can be avoided by defining WARN traps for the unsupported
I/O methods in your architecture.

Alan

Re: [PATCH 1/3] tty: serial: Use hub6_serial_X when CONFIG_SERIAL_8250_HUB6 is defined

[Alan Cox]

Most of these are not needed

> +#if CONFIG_SERIAL_8250_HUB6
>  static unsigned int hub6_serial_in(struct uart_port *p, int offset)
>  {
>  	offset = map_8250_in_reg(p, offset) << p->regshift;
> @@ -400,6 +401,7 @@ static void hub6_serial_out(struct uart_port *p, int offset, int value)
>  	outb(p->hub6 - 1 + offset, p->iobase);
>  	outb(value, p->iobase + 1);
>  }
> +#endif

This one is needed 
>  
>  static unsigned int mem_serial_in(struct uart_port *p, int offset)
>  {
> @@ -508,10 +510,12 @@ static void set_io_from_upio(struct uart_port *p)
>  	struct uart_8250_port *up =
>  		container_of(p, struct uart_8250_port, port);
>  	switch (p->iotype) {
> +#if CONFIG_SERIAL_8250_HUB6
>  	case UPIO_HUB6:
>  		p->serial_in = hub6_serial_in;
>  		p->serial_out = hub6_serial_out;
>  		break;
> +#endif

And this - but not the rest.

It is also such a miniscule amount of code it seems like it does not
justify the complexity of being configurable this way.

Re: [PATCH 1/3] tty: serial: Use hub6_serial_X when CONFIG_SERIAL_8250_HUB6 is defined

[Arnd Bergmann]

On Friday 11 March 2011, Alan Cox wrote:
> Most of these are not needed
> 
> It is also such a miniscule amount of code it seems like it does not
> justify the complexity of being configurable this way.

The point is that the 8250 driver stands in the way of allowing
to build the kernel on architectures that do not support ISA or PCI
I/O spaces. Right now, the common solution is to do

#define inb(x) readb(void __iomem *)(x))

or some variation of this. It's fine as long as this code never gets
called, but incorrect nonetheless.

I think it would be much cleaner if architectures that cannot do this
would not have to define those functions and we could make sure that
all drivers that do inb() have correct Kconfig dependencies.

	Arnd

Re: [PATCH 1/3] tty: serial: Use hub6_serial_X when CONFIG_SERIAL_8250_HUB6 is defined

[Alan Cox]

> or some variation of this. It's fine as long as this code never gets
> called, but incorrect nonetheless.

I disagree - the WARN(1) is certainly correct.

> I think it would be much cleaner if architectures that cannot do this
> would not have to define those functions and we could make sure that
> all drivers that do inb() have correct Kconfig dependencies.

For 8250 the way to do that is to remove all the switches and port type
stuff and propogate to setting ->serial_in and ->serial_out rather than
splattering the code with ifdefs. At that point you'd have a "lib8250" or
similar and an 8250_io/pci driver.

Alan

Re: [PATCH 1/3] tty: serial: Use hub6_serial_X when CONFIG_SERIAL_8250_HUB6 is defined

[Arnd Bergmann]

On Friday 11 March 2011, Alan Cox wrote:
> > or some variation of this. It's fine as long as this code never gets
> > called, but incorrect nonetheless.
> 
> I disagree - the WARN(1) is certainly correct.

Yes. I wrote this before I saw your other reply where you suggest
the use of WARN_ON().

So while technically correct, I still tend to prefer build warnings
over run-time warnings for things that we know about at build time.

> > I think it would be much cleaner if architectures that cannot do this
> > would not have to define those functions and we could make sure that
> > all drivers that do inb() have correct Kconfig dependencies.
> 
> For 8250 the way to do that is to remove all the switches and port type
> stuff and propogate to setting ->serial_in and ->serial_out rather than
> splattering the code with ifdefs. At that point you'd have a "lib8250" or
> similar and an 8250_io/pci driver.

Right, this absolutely makes sense. It's a lot more work than the originally
suggested patch, but the result is much cleaner.

	Arnd