2011-06-27 21:46:19

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH 0/7] serial/8250: I/O accessor cleanups

Hi Greg,

This series of patches cleans up the part of the 8250 device
driver that is responsible for accessing the hardware registers.
The driver defines all sorts of methods to do that right now,
when it really should only support memory mapped and programmed
I/O by default, and the latter only on PC-compatible platforms
including those that have ISA/PCMCIA/PCI buses.

The series shrinks the 8250 driver by about 10% in both binary
and source code size, hopefully with no loss of functionality,
and it allows platforms to no longer define bogus inb/outb
functions when they don't provide CONFIG_HAS_IOPORT. I've
build-tested for x86 and ARM with and without HAS_IOPORT.

Hopefully Ralf can provide some feedback about the three
MIPS platforms that have code changed by this.

Arnd

Arnd Bergmann (7):
serial/8250: remove obsolete RM9000 port type
serial/8250: move alchemy I/O handler to platform code
serial/8250: move UPIO_TSI to powerpc
serial/8250: move DWAP support to arch/mips
serial/8250: remove obsolete and broken PORT_RSA support
serial/8250: sanitize fourport handling
serial/8250: make PIO support optional

arch/mips/Kconfig | 7 -
arch/mips/alchemy/common/platform.c | 50 +++
arch/mips/pmc-sierra/msp71xx/msp_serial.c | 32 ++-
arch/powerpc/kernel/legacy_serial.c | 24 ++
drivers/tty/serial/8250.c | 472 +++--------------------------
drivers/tty/serial/8250.h | 8 +
drivers/tty/serial/8250_hub6.c | 17 +
drivers/tty/serial/Kconfig | 15 -
drivers/tty/serial/serial_core.c | 4 -
drivers/tty/serial/sunsu.c | 93 ------
include/linux/serial.h | 2 +-
include/linux/serial_core.h | 10 +-
include/linux/serial_reg.h | 51 ---
13 files changed, 171 insertions(+), 614 deletions(-)

Cc: Ralf Baechle <[email protected]>
Cc: [email protected]
Cc: Benjamin Herrenschmidt <[email protected]>

--
1.7.5.4


2011-06-27 21:47:29

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH 1/7] serial/8250: remove obsolete RM9000 port type

This code was only used on the MIPS eXcite platform, which got
removed in 2009. Since no code ever sets UPIO_RM9000, none of
the code is being used any more now.

Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Thomas Koeller <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
---
arch/mips/Kconfig | 7 ----
drivers/tty/serial/8250.c | 68 +------------------------------------------
drivers/tty/serial/Kconfig | 9 ------
include/linux/serial_core.h | 3 +-
4 files changed, 2 insertions(+), 85 deletions(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 653da62..a07f7b8 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -1045,10 +1045,6 @@ config PCI_GT64XXX_PCI0
config NO_EXCEPT_FILL
bool

-config MIPS_RM9122
- bool
- select SERIAL_RM9000
-
config SOC_EMMA2RH
bool
select CEVT_R4K
@@ -1094,9 +1090,6 @@ config SOC_PNX8550
config SWAP_IO_SPACE
bool

-config SERIAL_RM9000
- bool
-
config SGI_HAS_INDYDOG
bool

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index b40f7b9..cf19b26 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -273,13 +273,6 @@ static const struct serial8250_config uart_config[] = {
.fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
.flags = UART_CAP_FIFO | UART_CAP_UUE | UART_CAP_RTOIE,
},
- [PORT_RM9000] = {
- .name = "RM9000",
- .fifo_size = 16,
- .tx_loadsz = 16,
- .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
- .flags = UART_CAP_FIFO,
- },
[PORT_OCTEON] = {
.name = "OCTEON",
.fifo_size = 64,
@@ -347,44 +340,6 @@ static inline int map_8250_out_reg(struct uart_port *p, int offset)
return au_io_out_map[offset];
}

-#elif defined(CONFIG_SERIAL_8250_RM9K)
-
-static const u8
- regmap_in[8] = {
- [UART_RX] = 0x00,
- [UART_IER] = 0x0c,
- [UART_IIR] = 0x14,
- [UART_LCR] = 0x1c,
- [UART_MCR] = 0x20,
- [UART_LSR] = 0x24,
- [UART_MSR] = 0x28,
- [UART_SCR] = 0x2c
- },
- regmap_out[8] = {
- [UART_TX] = 0x04,
- [UART_IER] = 0x0c,
- [UART_FCR] = 0x18,
- [UART_LCR] = 0x1c,
- [UART_MCR] = 0x20,
- [UART_LSR] = 0x24,
- [UART_MSR] = 0x28,
- [UART_SCR] = 0x2c
- };
-
-static inline int map_8250_in_reg(struct uart_port *p, int offset)
-{
- if (p->iotype != UPIO_RM9000)
- return offset;
- return regmap_in[offset];
-}
-
-static inline int map_8250_out_reg(struct uart_port *p, int offset)
-{
- if (p->iotype != UPIO_RM9000)
- return offset;
- return regmap_out[offset];
-}
-
#else

/* sane hardware needs no mapping */
@@ -524,7 +479,6 @@ static void set_io_from_upio(struct uart_port *p)
p->serial_out = mem_serial_out;
break;

- case UPIO_RM9000:
case UPIO_MEM32:
p->serial_in = mem32_serial_in;
p->serial_out = mem32_serial_out;
@@ -620,24 +574,6 @@ static void serial_dl_write(struct uart_8250_port *up, int value)
else
_serial_dl_write(up, value);
}
-#elif defined(CONFIG_SERIAL_8250_RM9K)
-static int serial_dl_read(struct uart_8250_port *up)
-{
- return (up->port.iotype == UPIO_RM9000) ?
- (((__raw_readl(up->port.membase + 0x10) << 8) |
- (__raw_readl(up->port.membase + 0x08) & 0xff)) & 0xffff) :
- _serial_dl_read(up);
-}
-
-static void serial_dl_write(struct uart_8250_port *up, int value)
-{
- if (up->port.iotype == UPIO_RM9000) {
- __raw_writel(value, up->port.membase + 0x08);
- __raw_writel(value >> 8, up->port.membase + 0x10);
- } else {
- _serial_dl_write(up, value);
- }
-}
#else
#define serial_dl_read(up) _serial_dl_read(up)
#define serial_dl_write(up, value) _serial_dl_write(up, value)
@@ -1394,9 +1330,7 @@ static void serial8250_start_tx(struct uart_port *port)
unsigned char lsr;
lsr = serial_in(up, UART_LSR);
up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
- if ((up->port.type == PORT_RM9000) ?
- (lsr & UART_LSR_THRE) :
- (lsr & UART_LSR_TEMT))
+ if (lsr & UART_LSR_TEMT)
transmit_chars(up);
}
}
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 636144c..ea12815 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -258,15 +258,6 @@ config SERIAL_8250_ACORN
system, say Y to this option. The driver can handle 1, 2, or 3 port
cards. If unsure, say N.

-config SERIAL_8250_RM9K
- bool "Support for MIPS RM9xxx integrated serial port"
- depends on SERIAL_8250 != n && SERIAL_RM9000
- select SERIAL_8250_SHARE_IRQ
- help
- Selecting this option will add support for the integrated serial
- port hardware found on MIPS RM9122 and similar processors.
- If unsure, say N.
-
comment "Non-8250 serial port support"

config SERIAL_AMBA_PL010
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index a5c3114..df295e0 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -41,7 +41,7 @@
#define PORT_RSA 13
#define PORT_NS16550A 14
#define PORT_XSCALE 15
-#define PORT_RM9000 16 /* PMC-Sierra RM9xxx internal UART */
+/* #define PORT_RM9000 16 */ /* was: PMC-Sierra RM9xxx internal UART */
#define PORT_OCTEON 17 /* Cavium OCTEON internal UART */
#define PORT_AR7 18 /* Texas Instruments AR7 internal UART */
#define PORT_U6_16550A 19 /* ST-Ericsson U6xxx internal UART */
@@ -318,7 +318,6 @@ struct uart_port {
#define UPIO_AU (4) /* Au1x00 type IO */
#define UPIO_TSI (5) /* Tsi108/109 type IO */
#define UPIO_DWAPB (6) /* DesignWare APB UART */
-#define UPIO_RM9000 (7) /* RM9000 type IO */
#define UPIO_DWAPB32 (8) /* DesignWare APB UART (32 bit accesses) */

unsigned int read_status_mask; /* driver specific */
--
1.7.5.4

2011-06-27 21:47:14

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH 2/7] serial/8250: move alchemy I/O handler to platform code

Only one platform ever sets the UPIO_AU iotype, so it's
cleaner to define the handlers in the code that actually
requires it, rather than building the same logic into
every 8250 driver.

Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Manuel Lauss <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
---
arch/mips/alchemy/common/platform.c | 50 ++++++++++++++++++++++++++++++
drivers/tty/serial/8250.c | 58 -----------------------------------
2 files changed, 50 insertions(+), 58 deletions(-)

diff --git a/arch/mips/alchemy/common/platform.c b/arch/mips/alchemy/common/platform.c
index 3b2c18b..750441f 100644
--- a/arch/mips/alchemy/common/platform.c
+++ b/arch/mips/alchemy/common/platform.c
@@ -45,6 +45,54 @@ static void alchemy_8250_pm(struct uart_port *port, unsigned int state,
#endif
}

+/* Au1x00 UART hardware has a weird register layout */
+static const u8 au_io_in_map[] = {
+ [UART_RX] = 0,
+ [UART_IER] = 2,
+ [UART_IIR] = 3,
+ [UART_LCR] = 5,
+ [UART_MCR] = 6,
+ [UART_LSR] = 7,
+ [UART_MSR] = 8,
+};
+
+static const u8 au_io_out_map[] = {
+ [UART_TX] = 1,
+ [UART_IER] = 2,
+ [UART_FCR] = 4,
+ [UART_LCR] = 5,
+ [UART_MCR] = 6,
+};
+
+/* sane hardware needs no mapping */
+static inline int map_8250_in_reg(struct uart_port *p, int offset)
+{
+ if (p->iotype != UPIO_AU)
+ return offset;
+ return au_io_in_map[offset];
+}
+
+static inline int map_8250_out_reg(struct uart_port *p, int offset)
+{
+ if (p->iotype != UPIO_AU)
+ return offset;
+ return au_io_out_map[offset];
+}
+
+/* sane hardware needs no mapping */
+static unsigned int au_serial_in(struct uart_port *p, int offset)
+{
+ offset = map_8250_in_reg(p, offset) << p->regshift;
+ return __raw_readl(p->membase + offset);
+}
+
+static void au_serial_out(struct uart_port *p, int offset, int value)
+{
+ offset = map_8250_out_reg(p, offset) << p->regshift;
+ __raw_writel(value, p->membase + offset);
+}
+
+
#define PORT(_base, _irq) \
{ \
.mapbase = _base, \
@@ -55,6 +103,8 @@ static void alchemy_8250_pm(struct uart_port *port, unsigned int state,
UPF_FIXED_TYPE, \
.type = PORT_16550A, \
.pm = alchemy_8250_pm, \
+ .serial_in = au_serial_in, \
+ .serial_out = au_serial_out \
}

static struct plat_serial8250_port au1x00_uart_data[][4] __initdata = {
diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index cf19b26..c8f107e 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -304,50 +304,9 @@ static const struct serial8250_config uart_config[] = {
},
};

-#if defined(CONFIG_MIPS_ALCHEMY)
-
-/* Au1x00 UART hardware has a weird register layout */
-static const u8 au_io_in_map[] = {
- [UART_RX] = 0,
- [UART_IER] = 2,
- [UART_IIR] = 3,
- [UART_LCR] = 5,
- [UART_MCR] = 6,
- [UART_LSR] = 7,
- [UART_MSR] = 8,
-};
-
-static const u8 au_io_out_map[] = {
- [UART_TX] = 1,
- [UART_IER] = 2,
- [UART_FCR] = 4,
- [UART_LCR] = 5,
- [UART_MCR] = 6,
-};
-
-/* sane hardware needs no mapping */
-static inline int map_8250_in_reg(struct uart_port *p, int offset)
-{
- if (p->iotype != UPIO_AU)
- return offset;
- return au_io_in_map[offset];
-}
-
-static inline int map_8250_out_reg(struct uart_port *p, int offset)
-{
- if (p->iotype != UPIO_AU)
- return offset;
- return au_io_out_map[offset];
-}
-
-#else
-
-/* sane hardware needs no mapping */
#define map_8250_in_reg(up, offset) (offset)
#define map_8250_out_reg(up, offset) (offset)

-#endif
-
static unsigned int hub6_serial_in(struct uart_port *p, int offset)
{
offset = map_8250_in_reg(p, offset) << p->regshift;
@@ -386,18 +345,6 @@ static unsigned int mem32_serial_in(struct uart_port *p, int offset)
return readl(p->membase + offset);
}

-static unsigned int au_serial_in(struct uart_port *p, int offset)
-{
- offset = map_8250_in_reg(p, offset) << p->regshift;
- return __raw_readl(p->membase + offset);
-}
-
-static void au_serial_out(struct uart_port *p, int offset, int value)
-{
- offset = map_8250_out_reg(p, offset) << p->regshift;
- __raw_writel(value, p->membase + offset);
-}
-
static unsigned int tsi_serial_in(struct uart_port *p, int offset)
{
unsigned int tmp;
@@ -484,11 +431,6 @@ static void set_io_from_upio(struct uart_port *p)
p->serial_out = mem32_serial_out;
break;

- case UPIO_AU:
- p->serial_in = au_serial_in;
- p->serial_out = au_serial_out;
- break;
-
case UPIO_TSI:
p->serial_in = tsi_serial_in;
p->serial_out = tsi_serial_out;
--
1.7.5.4

2011-06-27 21:47:41

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH 3/7] serial/8250: move UPIO_TSI to powerpc

This iotype is only used by the legacy_serial code in powerpc, so the
code should live there, rather than be compiled in for every 8250
driver.

Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: [email protected]
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
---
arch/powerpc/kernel/legacy_serial.c | 24 ++++++++++++++++++++++++
drivers/tty/serial/8250.c | 23 -----------------------
2 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c
index 2b97b80..b229e1e 100644
--- a/arch/powerpc/kernel/legacy_serial.c
+++ b/arch/powerpc/kernel/legacy_serial.c
@@ -47,6 +47,24 @@ static struct __initdata of_device_id legacy_serial_parents[] = {
static unsigned int legacy_serial_count;
static int legacy_serial_console = -1;

+static unsigned int tsi_serial_in(struct uart_port *p, int offset)
+{
+ unsigned int tmp;
+ offset = offset << p->regshift;
+ if (offset == UART_IIR) {
+ tmp = readl(p->membase + (UART_IIR & ~3));
+ return (tmp >> 16) & 0xff; /* UART_IIR % 4 == 2 */
+ } else
+ return readb(p->membase + offset);
+}
+
+static void tsi_serial_out(struct uart_port *p, int offset, int value)
+{
+ offset = offset << p->regshift;
+ if (!((offset == UART_IER) && (value & UART_IER_UUE)))
+ writeb(value, p->membase + offset);
+}
+
static int __init add_legacy_port(struct device_node *np, int want_index,
int iotype, phys_addr_t base,
phys_addr_t taddr, unsigned long irq,
@@ -102,6 +120,7 @@ static int __init add_legacy_port(struct device_node *np, int want_index,
legacy_serial_ports[index].iobase = base;
else
legacy_serial_ports[index].mapbase = base;
+
legacy_serial_ports[index].iotype = iotype;
legacy_serial_ports[index].uartclk = clock;
legacy_serial_ports[index].irq = irq;
@@ -112,6 +131,11 @@ static int __init add_legacy_port(struct device_node *np, int want_index,
legacy_serial_infos[index].speed = spd ? be32_to_cpup(spd) : 0;
legacy_serial_infos[index].irq_check_parent = irq_check_parent;

+ if (iotype == UPIO_TSI) {
+ legacy_serial_ports[index].serial_in = tsi_serial_in;
+ legacy_serial_ports[index].serial_out = tsi_serial_out;
+ }
+
printk(KERN_DEBUG "Found legacy serial port %d for %s\n",
index, np->full_name);
printk(KERN_DEBUG " %s=%llx, taddr=%llx, irq=%lx, clk=%d, speed=%d\n",
diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index c8f107e..d575ccb 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -345,24 +345,6 @@ static unsigned int mem32_serial_in(struct uart_port *p, int offset)
return readl(p->membase + offset);
}

-static unsigned int tsi_serial_in(struct uart_port *p, int offset)
-{
- unsigned int tmp;
- offset = map_8250_in_reg(p, offset) << p->regshift;
- if (offset == UART_IIR) {
- tmp = readl(p->membase + (UART_IIR & ~3));
- return (tmp >> 16) & 0xff; /* UART_IIR % 4 == 2 */
- } else
- return readb(p->membase + offset);
-}
-
-static void tsi_serial_out(struct uart_port *p, int offset, int value)
-{
- offset = map_8250_out_reg(p, offset) << p->regshift;
- if (!((offset == UART_IER) && (value & UART_IER_UUE)))
- writeb(value, p->membase + offset);
-}
-
/* Save the LCR value so it can be re-written when a Busy Detect IRQ occurs. */
static inline void dwapb_save_out_value(struct uart_port *p, int offset,
int value)
@@ -431,11 +413,6 @@ static void set_io_from_upio(struct uart_port *p)
p->serial_out = mem32_serial_out;
break;

- case UPIO_TSI:
- p->serial_in = tsi_serial_in;
- p->serial_out = tsi_serial_out;
- break;
-
case UPIO_DWAPB:
p->serial_in = mem_serial_in;
p->serial_out = dwapb_serial_out;
--
1.7.5.4

2011-06-27 21:46:24

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH 4/7] serial/8250: move DWAP support to arch/mips

Only one board uses the UPIO_DWAP iotype, and nothing uses
the UPIO_DWAP32 iotype. It seems much cleaner to handle the
DWAP quirk as a new port type, and move the serial_out
accessor into the platform code. If more platforms start
using the DWAP port, that function can be moved into a
common location.

Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Jamie Iles <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
---
arch/mips/pmc-sierra/msp71xx/msp_serial.c | 32 ++++++++++-
drivers/tty/serial/8250.c | 87 +++--------------------------
drivers/tty/serial/8250_hub6.c | 17 ++++++
drivers/tty/serial/serial_core.c | 4 -
include/linux/serial_core.h | 5 +-
5 files changed, 57 insertions(+), 88 deletions(-)

diff --git a/arch/mips/pmc-sierra/msp71xx/msp_serial.c b/arch/mips/pmc-sierra/msp71xx/msp_serial.c
index f726162..6c9f1d3 100644
--- a/arch/mips/pmc-sierra/msp71xx/msp_serial.c
+++ b/arch/mips/pmc-sierra/msp71xx/msp_serial.c
@@ -38,6 +38,33 @@
#include <msp_int.h>
#include <msp_regs.h>

+/* Save the LCR value so it can be re-written when a Busy Detect IRQ occurs. */
+static inline void dwapb_save_out_value(struct uart_port *p, int offset,
+ int value)
+{
+ struct uart_8250_port *up =
+ container_of(p, struct uart_8250_port, port);
+
+ if (offset == UART_LCR)
+ up->lcr = value;
+}
+
+/* Read the IER to ensure any interrupt is cleared before returning from ISR. */
+static inline void dwapb_check_clear_ier(struct uart_port *p, int offset)
+{
+ if (offset == UART_TX || offset == UART_IER)
+ p->serial_in(p, UART_IER);
+}
+
+static void dwapb_serial_out(struct uart_port *p, int offset, int value)
+{
+ int save_offset = offset;
+ offset = map_8250_out_reg(p, offset) << p->regshift;
+ dwapb_save_out_value(p, save_offset, value);
+ writeb(value, p->membase + offset);
+ dwapb_check_clear_ier(p, save_offset);
+}
+
void __init msp_serial_setup(void)
{
char *s;
@@ -59,9 +86,10 @@ void __init msp_serial_setup(void)
up.irq = MSP_INT_UART0;
up.uartclk = uartclk;
up.regshift = 2;
- up.iotype = UPIO_DWAPB; /* UPIO_MEM like */
+ up.iotype = UPIO_MEM;
+ up.serial_out = dwapb_serial_out;
up.flags = ASYNC_BOOT_AUTOCONF | ASYNC_SKIP_TEST;
- up.type = PORT_16550A;
+ up.type = PORT_DWAP;
up.line = 0;
up.private_data = (void*)UART0_STATUS_REG;
if (early_serial_setup(&up))
diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index d575ccb..5698157 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -100,12 +100,6 @@ static unsigned int skip_txen_test; /* force skip of txen test at init time */
#define CONFIG_SERIAL_MANY_PORTS 1
#endif

-/*
- * HUB6 is always on. This will be removed once the header
- * files have been cleaned.
- */
-#define CONFIG_HUB6 1
-
#include <asm/serial.h>
/*
* SERIAL_PORT_DFNS tells us about built-in ports that have no
@@ -302,25 +296,18 @@ static const struct serial8250_config uart_config[] = {
UART_FCR_T_TRIG_01,
.flags = UART_CAP_FIFO | UART_CAP_RTOIE,
},
+ [PORT_DWAP] = {
+ .name = "16550A",
+ .fifo_size = 16,
+ .tx_loadsz = 16,
+ .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
+ .flags = UART_CAP_FIFO,
+ },
};

#define map_8250_in_reg(up, offset) (offset)
#define map_8250_out_reg(up, offset) (offset)

-static unsigned int hub6_serial_in(struct uart_port *p, int offset)
-{
- offset = map_8250_in_reg(p, offset) << p->regshift;
- outb(p->hub6 - 1 + offset, p->iobase);
- return inb(p->iobase + 1);
-}
-
-static void hub6_serial_out(struct uart_port *p, int offset, int value)
-{
- offset = map_8250_out_reg(p, offset) << p->regshift;
- outb(p->hub6 - 1 + offset, p->iobase);
- outb(value, p->iobase + 1);
-}
-
static unsigned int mem_serial_in(struct uart_port *p, int offset)
{
offset = map_8250_in_reg(p, offset) << p->regshift;
@@ -345,42 +332,6 @@ static unsigned int mem32_serial_in(struct uart_port *p, int offset)
return readl(p->membase + offset);
}

-/* Save the LCR value so it can be re-written when a Busy Detect IRQ occurs. */
-static inline void dwapb_save_out_value(struct uart_port *p, int offset,
- int value)
-{
- struct uart_8250_port *up =
- container_of(p, struct uart_8250_port, port);
-
- if (offset == UART_LCR)
- up->lcr = value;
-}
-
-/* Read the IER to ensure any interrupt is cleared before returning from ISR. */
-static inline void dwapb_check_clear_ier(struct uart_port *p, int offset)
-{
- if (offset == UART_TX || offset == UART_IER)
- p->serial_in(p, UART_IER);
-}
-
-static void dwapb_serial_out(struct uart_port *p, int offset, int value)
-{
- int save_offset = offset;
- offset = map_8250_out_reg(p, offset) << p->regshift;
- dwapb_save_out_value(p, save_offset, value);
- writeb(value, p->membase + offset);
- dwapb_check_clear_ier(p, save_offset);
-}
-
-static void dwapb32_serial_out(struct uart_port *p, int offset, int value)
-{
- int save_offset = offset;
- offset = map_8250_out_reg(p, offset) << p->regshift;
- dwapb_save_out_value(p, save_offset, value);
- writel(value, p->membase + offset);
- dwapb_check_clear_ier(p, save_offset);
-}
-
static unsigned int io_serial_in(struct uart_port *p, int offset)
{
offset = map_8250_in_reg(p, offset) << p->regshift;
@@ -398,11 +349,6 @@ 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) {
- case UPIO_HUB6:
- p->serial_in = hub6_serial_in;
- p->serial_out = hub6_serial_out;
- break;
-
case UPIO_MEM:
p->serial_in = mem_serial_in;
p->serial_out = mem_serial_out;
@@ -413,16 +359,6 @@ static void set_io_from_upio(struct uart_port *p)
p->serial_out = mem32_serial_out;
break;

- case UPIO_DWAPB:
- p->serial_in = mem_serial_in;
- p->serial_out = dwapb_serial_out;
- break;
-
- case UPIO_DWAPB32:
- p->serial_in = mem32_serial_in;
- p->serial_out = dwapb32_serial_out;
- break;
-
default:
p->serial_in = io_serial_in;
p->serial_out = io_serial_out;
@@ -440,8 +376,6 @@ serial_out_sync(struct uart_8250_port *up, int offset, int value)
case UPIO_MEM:
case UPIO_MEM32:
case UPIO_AU:
- case UPIO_DWAPB:
- case UPIO_DWAPB32:
p->serial_out(p, offset, value);
p->serial_in(p, UART_LCR); /* safe, no side-effects */
break;
@@ -1512,8 +1446,7 @@ static irqreturn_t serial8250_interrupt(int irq, void *dev_id)
handled = 1;

end = NULL;
- } else if ((up->port.iotype == UPIO_DWAPB ||
- up->port.iotype == UPIO_DWAPB32) &&
+ } else if ((up->port.type == PORT_DWAP) &&
(iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
/* The DesignWare APB UART has an Busy Detect (0x07)
* interrupt meaning an LCR write attempt occurred while the
@@ -2421,8 +2354,6 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
case UPIO_TSI:
case UPIO_MEM32:
case UPIO_MEM:
- case UPIO_DWAPB:
- case UPIO_DWAPB32:
if (!up->port.mapbase)
break;

@@ -2459,8 +2390,6 @@ static void serial8250_release_std_resource(struct uart_8250_port *up)
case UPIO_TSI:
case UPIO_MEM32:
case UPIO_MEM:
- case UPIO_DWAPB:
- case UPIO_DWAPB32:
if (!up->port.mapbase)
break;

diff --git a/drivers/tty/serial/8250_hub6.c b/drivers/tty/serial/8250_hub6.c
index a5c778e..fd1361e 100644
--- a/drivers/tty/serial/8250_hub6.c
+++ b/drivers/tty/serial/8250_hub6.c
@@ -9,6 +9,21 @@
#include <linux/module.h>
#include <linux/init.h>
#include <linux/serial_8250.h>
+#include <linux/io.h>
+
+static unsigned int hub6_serial_in(struct uart_port *p, int offset)
+{
+ offset = offset << p->regshift;
+ outb(p->hub6 - 1 + offset, p->iobase);
+ return inb(p->iobase + 1);
+}
+
+static void hub6_serial_out(struct uart_port *p, int offset, int value)
+{
+ offset = offset << p->regshift;
+ outb(p->hub6 - 1 + offset, p->iobase);
+ outb(value, p->iobase + 1);
+}

#define HUB6(card,port) \
{ \
@@ -18,6 +33,8 @@
.iotype = UPIO_HUB6, \
.flags = UPF_BOOT_AUTOCONF, \
.hub6 = (card) << 6 | (port) << 3 | 1, \
+ .serial_in = hub6_serial_in, \
+ .serial_out = hub6_serial_out, \
}

static struct plat_serial8250_port hub6_data[] = {
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index db7912c..e5a0cae 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2063,8 +2063,6 @@ uart_report_port(struct uart_driver *drv, struct uart_port *port)
case UPIO_MEM32:
case UPIO_AU:
case UPIO_TSI:
- case UPIO_DWAPB:
- case UPIO_DWAPB32:
snprintf(address, sizeof(address),
"MMIO 0x%llx", (unsigned long long)port->mapbase);
break;
@@ -2484,8 +2482,6 @@ int uart_match_port(struct uart_port *port1, struct uart_port *port2)
case UPIO_MEM32:
case UPIO_AU:
case UPIO_TSI:
- case UPIO_DWAPB:
- case UPIO_DWAPB32:
return (port1->mapbase == port2->mapbase);
}
return 0;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index df295e0..598992f 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -46,7 +46,8 @@
#define PORT_AR7 18 /* Texas Instruments AR7 internal UART */
#define PORT_U6_16550A 19 /* ST-Ericsson U6xxx internal UART */
#define PORT_TEGRA 20 /* NVIDIA Tegra internal UART */
-#define PORT_MAX_8250 20 /* max port ID */
+#define PORT_DWAP 21 /* Designware */
+#define PORT_MAX_8250 21 /* max port ID */

/*
* ARM specific type numbers. These are not currently guaranteed
@@ -317,8 +318,6 @@ struct uart_port {
#define UPIO_MEM32 (3)
#define UPIO_AU (4) /* Au1x00 type IO */
#define UPIO_TSI (5) /* Tsi108/109 type IO */
-#define UPIO_DWAPB (6) /* DesignWare APB UART */
-#define UPIO_DWAPB32 (8) /* DesignWare APB UART (32 bit accesses) */

unsigned int read_status_mask; /* driver specific */
unsigned int ignore_status_mask; /* driver specific */
--
1.7.5.4

2011-06-27 21:47:36

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH 5/7] serial/8250: remove obsolete and broken PORT_RSA support

For all I can tell, the RSA support in the 8250 driver
is not functional and has not been for a long time.

The RSA-DV II/S ISA card[1] was sold by IODATA in the
1990s as a faster alternative to regular ISA serial ports,
and it was discontinued in 2002. Linux support was added
by Kiyokazu SUTO in 2000.

The driver defines an I/O port base address of -8 (negative
eight), which should always get refused by request_region
on all architectures for being outside of the valid
resource range.

Further, it seems that a patch to setserial would be
required in order to set PORT_RSA when not using
autoconfiguration.

This patch removes all code related to the RSA support, with
the exception of the user-visible PORT_RSA definition.
Some of the code has been copied into the sunsu.c driver,
so remove it from there as well.

[1] http://translate.google.com/translate?hl=en&sl=ja&u=http://www.iodata.jp/products/network/rsadv2s.htm

Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Kiyokazu SUTO <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
---
drivers/tty/serial/8250.c | 200 ++-----------------------------------------
drivers/tty/serial/Kconfig | 6 --
drivers/tty/serial/sunsu.c | 93 --------------------
include/linux/serial.h | 2 +-
include/linux/serial_core.h | 2 +-
include/linux/serial_reg.h | 51 -----------
6 files changed, 10 insertions(+), 344 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 5698157..0ddba34 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -116,13 +116,6 @@ static const struct old_serial_port old_serial_port[] = {

#define UART_NR CONFIG_SERIAL_8250_NR_UARTS

-#ifdef CONFIG_SERIAL_8250_RSA
-
-#define PORT_RSA_MAX 4
-static unsigned long probe_rsa[PORT_RSA_MAX];
-static unsigned int probe_rsa_count;
-#endif /* CONFIG_SERIAL_8250_RSA */
-
struct uart_8250_port {
struct uart_port port;
struct timer_list timer; /* "no irq" timer */
@@ -246,13 +239,6 @@ static const struct serial8250_config uart_config[] = {
.fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
.flags = UART_CAP_FIFO | UART_CAP_EFR | UART_CAP_SLEEP,
},
- [PORT_RSA] = {
- .name = "RSA",
- .fifo_size = 2048,
- .tx_loadsz = 2048,
- .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_11,
- .flags = UART_CAP_FIFO,
- },
[PORT_NS16550A] = {
.name = "NS16550A",
.fifo_size = 16,
@@ -488,75 +474,6 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
}
}

-#ifdef CONFIG_SERIAL_8250_RSA
-/*
- * Attempts to turn on the RSA FIFO. Returns zero on failure.
- * We set the port uart clock rate if we succeed.
- */
-static int __enable_rsa(struct uart_8250_port *up)
-{
- unsigned char mode;
- int result;
-
- mode = serial_inp(up, UART_RSA_MSR);
- result = mode & UART_RSA_MSR_FIFO;
-
- if (!result) {
- serial_outp(up, UART_RSA_MSR, mode | UART_RSA_MSR_FIFO);
- mode = serial_inp(up, UART_RSA_MSR);
- result = mode & UART_RSA_MSR_FIFO;
- }
-
- if (result)
- up->port.uartclk = SERIAL_RSA_BAUD_BASE * 16;
-
- return result;
-}
-
-static void enable_rsa(struct uart_8250_port *up)
-{
- if (up->port.type == PORT_RSA) {
- if (up->port.uartclk != SERIAL_RSA_BAUD_BASE * 16) {
- spin_lock_irq(&up->port.lock);
- __enable_rsa(up);
- spin_unlock_irq(&up->port.lock);
- }
- if (up->port.uartclk == SERIAL_RSA_BAUD_BASE * 16)
- serial_outp(up, UART_RSA_FRR, 0);
- }
-}
-
-/*
- * Attempts to turn off the RSA FIFO. Returns zero on failure.
- * It is unknown why interrupts were disabled in here. However,
- * the caller is expected to preserve this behaviour by grabbing
- * the spinlock before calling this function.
- */
-static void disable_rsa(struct uart_8250_port *up)
-{
- unsigned char mode;
- int result;
-
- if (up->port.type == PORT_RSA &&
- up->port.uartclk == SERIAL_RSA_BAUD_BASE * 16) {
- spin_lock_irq(&up->port.lock);
-
- mode = serial_inp(up, UART_RSA_MSR);
- result = !(mode & UART_RSA_MSR_FIFO);
-
- if (!result) {
- serial_outp(up, UART_RSA_MSR, mode & ~UART_RSA_MSR_FIFO);
- mode = serial_inp(up, UART_RSA_MSR);
- result = !(mode & UART_RSA_MSR_FIFO);
- }
-
- if (result)
- up->port.uartclk = SERIAL_RSA_BAUD_BASE_LO * 16;
- spin_unlock_irq(&up->port.lock);
- }
-}
-#endif /* CONFIG_SERIAL_8250_RSA */
-
/*
* This is a quickie test to see how big the FIFO is.
* It doesn't work at all the time, more's the pity.
@@ -1042,23 +959,6 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
break;
}

-#ifdef CONFIG_SERIAL_8250_RSA
- /*
- * Only probe for RSA ports if we got the region.
- */
- if (up->port.type == PORT_16550A && probeflags & PROBE_RSA) {
- int i;
-
- for (i = 0 ; i < probe_rsa_count; ++i) {
- if (probe_rsa[i] == up->port.iobase &&
- __enable_rsa(up)) {
- up->port.type = PORT_RSA;
- break;
- }
- }
- }
-#endif
-
serial_outp(up, UART_LCR, save_lcr);

if (up->capabilities != uart_config[up->port.type].flags) {
@@ -1078,10 +978,6 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
/*
* Reset the UART.
*/
-#ifdef CONFIG_SERIAL_8250_RSA
- if (up->port.type == PORT_RSA)
- serial_outp(up, UART_RSA_FRR, 0);
-#endif
serial_outp(up, UART_MCR, save_mcr);
serial8250_clear_fifos(up);
serial_in(up, UART_RX);
@@ -1836,14 +1732,6 @@ static int serial8250_startup(struct uart_port *port)
serial_outp(up, UART_LCR, 0);
}

-#ifdef CONFIG_SERIAL_8250_RSA
- /*
- * If this is an RSA port, see if we can kick it up to the
- * higher speed clock.
- */
- enable_rsa(up);
-#endif
-
/*
* Clear the FIFO buffers and disable them.
* (they will be reenabled in set_termios())
@@ -2067,13 +1955,6 @@ static void serial8250_shutdown(struct uart_port *port)
serial_out(up, UART_LCR, serial_inp(up, UART_LCR) & ~UART_LCR_SBC);
serial8250_clear_fifos(up);

-#ifdef CONFIG_SERIAL_8250_RSA
- /*
- * Reset the RSA board back to 115kbps compat mode.
- */
- disable_rsa(up);
-#endif
-
/*
* Read data port to reset things, and then unlink from
* the IRQ chain.
@@ -2344,8 +2225,10 @@ static unsigned int serial8250_port_size(struct uart_8250_port *pt)
/*
* Resource handling.
*/
-static int serial8250_request_std_resource(struct uart_8250_port *up)
+static int serial8250_request_port(struct uart_port *port)
{
+ struct uart_8250_port *up =
+ container_of(port, struct uart_8250_port, port);
unsigned int size = serial8250_port_size(up);
int ret = 0;

@@ -2381,8 +2264,10 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
return ret;
}

-static void serial8250_release_std_resource(struct uart_8250_port *up)
+static void serial8250_release_port(struct uart_port *port)
{
+ struct uart_8250_port *up =
+ container_of(port, struct uart_8250_port, port);
unsigned int size = serial8250_port_size(up);

switch (up->port.iotype) {
@@ -2408,65 +2293,6 @@ static void serial8250_release_std_resource(struct uart_8250_port *up)
}
}

-static int serial8250_request_rsa_resource(struct uart_8250_port *up)
-{
- unsigned long start = UART_RSA_BASE << up->port.regshift;
- unsigned int size = 8 << up->port.regshift;
- int ret = -EINVAL;
-
- switch (up->port.iotype) {
- case UPIO_HUB6:
- 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)
-{
- unsigned long offset = UART_RSA_BASE << up->port.regshift;
- unsigned int size = 8 << up->port.regshift;
-
- switch (up->port.iotype) {
- case UPIO_HUB6:
- case UPIO_PORT:
- release_region(up->port.iobase + offset, size);
- break;
- }
-}
-
-static void serial8250_release_port(struct uart_port *port)
-{
- struct uart_8250_port *up =
- container_of(port, struct uart_8250_port, port);
-
- serial8250_release_std_resource(up);
- if (up->port.type == PORT_RSA)
- serial8250_release_rsa_resource(up);
-}
-
-static int serial8250_request_port(struct uart_port *port)
-{
- struct uart_8250_port *up =
- container_of(port, struct uart_8250_port, port);
- int ret = 0;
-
- ret = serial8250_request_std_resource(up);
- if (ret == 0 && up->port.type == PORT_RSA) {
- ret = serial8250_request_rsa_resource(up);
- if (ret < 0)
- serial8250_release_std_resource(up);
- }
-
- return ret;
-}
-
static void serial8250_config_port(struct uart_port *port, int flags)
{
struct uart_8250_port *up =
@@ -2478,14 +2304,10 @@ static void serial8250_config_port(struct uart_port *port, int flags)
* Find the region that we can probe for. This in turn
* tells us whether we can probe for the type of port.
*/
- ret = serial8250_request_std_resource(up);
+ ret = serial8250_request_port(port);
if (ret < 0)
return;

- ret = serial8250_request_rsa_resource(up);
- if (ret < 0)
- probeflags &= ~PROBE_RSA;
-
if (up->port.iotype != up->cur_iotype)
set_io_from_upio(port);

@@ -2499,10 +2321,8 @@ static void serial8250_config_port(struct uart_port *port, int flags)
if (up->port.type != PORT_UNKNOWN && flags & UART_CONFIG_IRQ)
autoconfig_irq(up);

- if (up->port.type != PORT_RSA && probeflags & PROBE_RSA)
- serial8250_release_rsa_resource(up);
if (up->port.type == PORT_UNKNOWN)
- serial8250_release_std_resource(up);
+ serial8250_release_port(port);
}

static int
@@ -3198,8 +3018,4 @@ MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" __MODULE_STR
module_param(skip_txen_test, uint, 0644);
MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");

-#ifdef CONFIG_SERIAL_8250_RSA
-module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
-MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
-#endif
MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index ea12815..3ec604f 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -235,12 +235,6 @@ config SERIAL_8250_DETECT_IRQ

If unsure, say N.

-config SERIAL_8250_RSA
- bool "Support RSA serial ports"
- depends on SERIAL_8250_EXTENDED
- help
- ::: To be written :::
-
config SERIAL_8250_MCA
tristate "Support 8250-type ports on MCA buses"
depends on SERIAL_8250 != n && MCA
diff --git a/drivers/tty/serial/sunsu.c b/drivers/tty/serial/sunsu.c
index 92aa545..a52af85 100644
--- a/drivers/tty/serial/sunsu.c
+++ b/drivers/tty/serial/sunsu.c
@@ -75,7 +75,6 @@ static const struct serial_uart_config uart_config[PORT_MAX_8250+1] = {
{ "16C950/954", 128, UART_CLEAR_FIFO | UART_USE_FIFO },
{ "ST16654", 64, UART_CLEAR_FIFO | UART_USE_FIFO | UART_STARTECH },
{ "XR16850", 128, UART_CLEAR_FIFO | UART_USE_FIFO | UART_STARTECH },
- { "RSA", 2048, UART_CLEAR_FIFO | UART_USE_FIFO }
};

struct uart_sunsu_port {
@@ -179,75 +178,6 @@ static unsigned int serial_icr_read(struct uart_sunsu_port *up, int offset)
}
#endif

-#ifdef CONFIG_SERIAL_8250_RSA
-/*
- * Attempts to turn on the RSA FIFO. Returns zero on failure.
- * We set the port uart clock rate if we succeed.
- */
-static int __enable_rsa(struct uart_sunsu_port *up)
-{
- unsigned char mode;
- int result;
-
- mode = serial_inp(up, UART_RSA_MSR);
- result = mode & UART_RSA_MSR_FIFO;
-
- if (!result) {
- serial_outp(up, UART_RSA_MSR, mode | UART_RSA_MSR_FIFO);
- mode = serial_inp(up, UART_RSA_MSR);
- result = mode & UART_RSA_MSR_FIFO;
- }
-
- if (result)
- up->port.uartclk = SERIAL_RSA_BAUD_BASE * 16;
-
- return result;
-}
-
-static void enable_rsa(struct uart_sunsu_port *up)
-{
- if (up->port.type == PORT_RSA) {
- if (up->port.uartclk != SERIAL_RSA_BAUD_BASE * 16) {
- spin_lock_irq(&up->port.lock);
- __enable_rsa(up);
- spin_unlock_irq(&up->port.lock);
- }
- if (up->port.uartclk == SERIAL_RSA_BAUD_BASE * 16)
- serial_outp(up, UART_RSA_FRR, 0);
- }
-}
-
-/*
- * Attempts to turn off the RSA FIFO. Returns zero on failure.
- * It is unknown why interrupts were disabled in here. However,
- * the caller is expected to preserve this behaviour by grabbing
- * the spinlock before calling this function.
- */
-static void disable_rsa(struct uart_sunsu_port *up)
-{
- unsigned char mode;
- int result;
-
- if (up->port.type == PORT_RSA &&
- up->port.uartclk == SERIAL_RSA_BAUD_BASE * 16) {
- spin_lock_irq(&up->port.lock);
-
- mode = serial_inp(up, UART_RSA_MSR);
- result = !(mode & UART_RSA_MSR_FIFO);
-
- if (!result) {
- serial_outp(up, UART_RSA_MSR, mode & ~UART_RSA_MSR_FIFO);
- mode = serial_inp(up, UART_RSA_MSR);
- result = !(mode & UART_RSA_MSR_FIFO);
- }
-
- if (result)
- up->port.uartclk = SERIAL_RSA_BAUD_BASE_LO * 16;
- spin_unlock_irq(&up->port.lock);
- }
-}
-#endif /* CONFIG_SERIAL_8250_RSA */
-
static inline void __stop_tx(struct uart_sunsu_port *p)
{
if (p->ier & UART_IER_THRI) {
@@ -626,14 +556,6 @@ static int sunsu_startup(struct uart_port *port)
serial_outp(up, UART_LCR, 0);
}

-#ifdef CONFIG_SERIAL_8250_RSA
- /*
- * If this is an RSA port, see if we can kick it up to the
- * higher speed clock.
- */
- enable_rsa(up);
-#endif
-
/*
* Clear the FIFO buffers and disable them.
* (they will be reenabled in set_termios())
@@ -748,13 +670,6 @@ static void sunsu_shutdown(struct uart_port *port)
UART_FCR_CLEAR_XMIT);
serial_outp(up, UART_FCR, 0);

-#ifdef CONFIG_SERIAL_8250_RSA
- /*
- * Reset the RSA board back to 115kbps compat mode.
- */
- disable_rsa(up);
-#endif
-
/*
* Read data port to reset things.
*/
@@ -810,10 +725,6 @@ sunsu_change_speed(struct uart_port *port, unsigned int cflag,
if (uart_config[up->port.type].flags & UART_USE_FIFO) {
if ((up->port.uartclk / quot) < (2400 * 16))
fcr = UART_FCR_ENABLE_FIFO | UART_FCR_TRIGGER_1;
-#ifdef CONFIG_SERIAL_8250_RSA
- else if (up->port.type == PORT_RSA)
- fcr = UART_FCR_ENABLE_FIFO | UART_FCR_TRIGGER_14;
-#endif
else
fcr = UART_FCR_ENABLE_FIFO | UART_FCR_TRIGGER_8;
}
@@ -1156,10 +1067,6 @@ static void sunsu_autoconfig(struct uart_sunsu_port *up)
/*
* Reset the UART.
*/
-#ifdef CONFIG_SERIAL_8250_RSA
- if (up->port.type == PORT_RSA)
- serial_outp(up, UART_RSA_FRR, 0);
-#endif
serial_outp(up, UART_MCR, save_mcr);
serial_outp(up, UART_FCR, (UART_FCR_ENABLE_FIFO |
UART_FCR_CLEAR_RCVR |
diff --git a/include/linux/serial.h b/include/linux/serial.h
index ef91406..7bf30a6 100644
--- a/include/linux/serial.h
+++ b/include/linux/serial.h
@@ -76,7 +76,7 @@ struct serial_struct {
#define PORT_16C950 10 /* Oxford Semiconductor */
#define PORT_16654 11
#define PORT_16850 12
-#define PORT_RSA 13 /* RSA-DV II/S card */
+#define PORT_RSA 13 /* obsolete RSA-DV II/S card */
#define PORT_MAX 13

#define SERIAL_IO_PORT 0
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 598992f..f84c560 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -38,7 +38,7 @@
#define PORT_16C950 10
#define PORT_16654 11
#define PORT_16850 12
-#define PORT_RSA 13
+/* #define PORT_RSA 13 */ /* was: RSA-DV II/S */
#define PORT_NS16550A 14
#define PORT_XSCALE 15
/* #define PORT_RM9000 16 */ /* was: PMC-Sierra RM9xxx internal UART */
diff --git a/include/linux/serial_reg.h b/include/linux/serial_reg.h
index c75bda3..4fa3d59 100644
--- a/include/linux/serial_reg.h
+++ b/include/linux/serial_reg.h
@@ -284,57 +284,6 @@
#define UART_ACR_ICRRD 0x40 /* ICR Read enable */
#define UART_ACR_ASREN 0x80 /* Additional status enable */

-
-
-/*
- * These definitions are for the RSA-DV II/S card, from
- *
- * Kiyokazu SUTO <[email protected]>
- */
-
-#define UART_RSA_BASE (-8)
-
-#define UART_RSA_MSR ((UART_RSA_BASE) + 0) /* I/O: Mode Select Register */
-
-#define UART_RSA_MSR_SWAP (1 << 0) /* Swap low/high 8 bytes in I/O port addr */
-#define UART_RSA_MSR_FIFO (1 << 2) /* Enable the external FIFO */
-#define UART_RSA_MSR_FLOW (1 << 3) /* Enable the auto RTS/CTS flow control */
-#define UART_RSA_MSR_ITYP (1 << 4) /* Level (1) / Edge triger (0) */
-
-#define UART_RSA_IER ((UART_RSA_BASE) + 1) /* I/O: Interrupt Enable Register */
-
-#define UART_RSA_IER_Rx_FIFO_H (1 << 0) /* Enable Rx FIFO half full int. */
-#define UART_RSA_IER_Tx_FIFO_H (1 << 1) /* Enable Tx FIFO half full int. */
-#define UART_RSA_IER_Tx_FIFO_E (1 << 2) /* Enable Tx FIFO empty int. */
-#define UART_RSA_IER_Rx_TOUT (1 << 3) /* Enable char receive timeout int */
-#define UART_RSA_IER_TIMER (1 << 4) /* Enable timer interrupt */
-
-#define UART_RSA_SRR ((UART_RSA_BASE) + 2) /* IN: Status Read Register */
-
-#define UART_RSA_SRR_Tx_FIFO_NEMP (1 << 0) /* Tx FIFO is not empty (1) */
-#define UART_RSA_SRR_Tx_FIFO_NHFL (1 << 1) /* Tx FIFO is not half full (1) */
-#define UART_RSA_SRR_Tx_FIFO_NFUL (1 << 2) /* Tx FIFO is not full (1) */
-#define UART_RSA_SRR_Rx_FIFO_NEMP (1 << 3) /* Rx FIFO is not empty (1) */
-#define UART_RSA_SRR_Rx_FIFO_NHFL (1 << 4) /* Rx FIFO is not half full (1) */
-#define UART_RSA_SRR_Rx_FIFO_NFUL (1 << 5) /* Rx FIFO is not full (1) */
-#define UART_RSA_SRR_Rx_TOUT (1 << 6) /* Character reception timeout occurred (1) */
-#define UART_RSA_SRR_TIMER (1 << 7) /* Timer interrupt occurred */
-
-#define UART_RSA_FRR ((UART_RSA_BASE) + 2) /* OUT: FIFO Reset Register */
-
-#define UART_RSA_TIVSR ((UART_RSA_BASE) + 3) /* I/O: Timer Interval Value Set Register */
-
-#define UART_RSA_TCR ((UART_RSA_BASE) + 4) /* OUT: Timer Control Register */
-
-#define UART_RSA_TCR_SWITCH (1 << 0) /* Timer on */
-
-/*
- * The RSA DSV/II board has two fixed clock frequencies. One is the
- * standard rate, and the other is 8 times faster.
- */
-#define SERIAL_RSA_BAUD_BASE (921600)
-#define SERIAL_RSA_BAUD_BASE_LO (SERIAL_RSA_BAUD_BASE / 8)
-
/*
* Extra serial register definitions for the internal UARTs
* in TI OMAP processors.
--
1.7.5.4

2011-06-27 21:47:32

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH 6/7] serial/8250: sanitize fourport handling

Support for AST fourport cards is always built into
the 8250 driver, even if CONFIG_SERIAL_8250_FOURPORT
is disabled. This introduces a set of macros for
accessing the special interrupt control register on
the fourport card, so that support is left out
when the option is disabled, without adding more #ifdef
lines to the driver itself.

Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
---
drivers/tty/serial/8250.c | 18 +++++++-----------
drivers/tty/serial/8250.h | 8 ++++++++
2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 0ddba34..2b06c27 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -995,15 +995,13 @@ 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;

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);
+ save_ICP = fourport_icp_in(up);
+ fourport_icp_out(up, 0x80);
+ (void) fourport_icp_in(up);
}

/* forget possible initially masked and pending IRQ */
@@ -1035,7 +1033,7 @@ static void autoconfig_irq(struct uart_8250_port *up)
serial_outp(up, UART_IER, save_ier);

if (up->port.flags & UPF_FOURPORT)
- outb_p(save_ICP, ICP);
+ fourport_icp_out(up, save_ICP);

up->port.irq = (irq > 0) ? irq : 0;
}
@@ -1914,13 +1912,11 @@ dont_test_tx_en:
serial_outp(up, UART_IER, up->ier);

if (up->port.flags & UPF_FOURPORT) {
- unsigned int icp;
/*
* Enable interrupts on the AST Fourport board
*/
- icp = (up->port.iobase & 0xfe0) | 0x01f;
- outb_p(0x80, icp);
- (void) inb_p(icp);
+ fourport_icp_out(up, 0x80);
+ (void) fourport_icp_in(up);
}

return 0;
@@ -1941,7 +1937,7 @@ static void serial8250_shutdown(struct uart_port *port)
spin_lock_irqsave(&up->port.lock, flags);
if (up->port.flags & UPF_FOURPORT) {
/* reset interrupts on the AST Fourport board */
- inb((up->port.iobase & 0xfe0) | 0x1f);
+ (void) fourport_icp_in(up);
up->port.mctrl |= TIOCM_OUT1;
} else
up->port.mctrl &= ~TIOCM_OUT2;
diff --git a/drivers/tty/serial/8250.h b/drivers/tty/serial/8250.h
index 6edf4a6..1fc46fe 100644
--- a/drivers/tty/serial/8250.h
+++ b/drivers/tty/serial/8250.h
@@ -60,6 +60,14 @@ struct serial8250_config {
#define SERIAL8250_SHARE_IRQS 0
#endif

+#if defined(CONFIG_SERIAL_8250_FOURPORT) || defined(CONFIG_SERIAL_8250_FOURPORT_MODULE)
+# define fourport_icp_in(up) inb_p((up->port.iobase & 0xfe0) | 0x01f)
+# define fourport_icp_out(up, v) outb_p(v, (up->port.iobase & 0xfe0) | 0x01f)
+#else
+# define fourport_icp_in(up) (0)
+# define fourport_icp_out(up, v) do { } while (0)
+#endif
+
#if defined(__alpha__) && !defined(CONFIG_PCI)
/*
* Digital did something really horribly wrong with the OUT1 and OUT2
--
1.7.5.4

2011-06-27 21:47:44

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH 7/7] serial/8250: make PIO support optional

It is not currently possible to build support for the 8250 UART
on architectures that don't define PC-style I/O accessors like
inb/outb. Most embedded systems without PCI don't actually have
PC-style I/O, so they should not have to make up their own
accessors.

This makes the PIO support in the 8250 driver completely
conditional on CONFIG_HAS_IOPORT so we can remove the bogus
definitions from all the places that only need them for 8250.

Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
---
drivers/tty/serial/8250.c | 20 ++++++++++++++------
1 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 2b06c27..a2933d6 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -318,6 +318,7 @@ static unsigned int mem32_serial_in(struct uart_port *p, int offset)
return readl(p->membase + offset);
}

+#ifdef CONFIG_HAS_IOPORT
static unsigned int io_serial_in(struct uart_port *p, int offset)
{
offset = map_8250_in_reg(p, offset) << p->regshift;
@@ -329,16 +330,19 @@ 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) {
- case UPIO_MEM:
- p->serial_in = mem_serial_in;
- p->serial_out = mem_serial_out;
+#ifdef CONFIG_HAS_IOPORT
+ case UPIO_PORT:
+ p->serial_in = io_serial_in;
+ p->serial_out = io_serial_out;
break;
+#endif

case UPIO_MEM32:
p->serial_in = mem32_serial_in;
@@ -346,9 +350,10 @@ static void set_io_from_upio(struct uart_port *p)
break;

default:
- p->serial_in = io_serial_in;
- p->serial_out = io_serial_out;
+ p->serial_in = mem_serial_in;
+ p->serial_out = mem_serial_out;
break;
+
}
/* Remember loaded iotype */
up->cur_iotype = p->iotype;
@@ -2250,12 +2255,13 @@ static int serial8250_request_port(struct uart_port *port)
}
}
break;
-
+#ifdef CONFIG_HAS_IOPORT
case UPIO_HUB6:
case UPIO_PORT:
if (!request_region(up->port.iobase, size, "serial"))
ret = -EBUSY;
break;
+#endif
}
return ret;
}
@@ -2282,10 +2288,12 @@ static void serial8250_release_port(struct uart_port *port)
release_mem_region(up->port.mapbase, size);
break;

+#ifdef CONFIG_HAS_IOPORT
case UPIO_HUB6:
case UPIO_PORT:
release_region(up->port.iobase, size);
break;
+#endif
}
}

--
1.7.5.4

2011-06-27 22:17:32

by Jamie Iles

[permalink] [raw]
Subject: Re: [PATCH 4/7] serial/8250: move DWAP support to arch/mips

Hi Arnd,

On Mon, Jun 27, 2011 at 11:45:17PM +0200, Arnd Bergmann wrote:
> Only one board uses the UPIO_DWAP iotype, and nothing uses
> the UPIO_DWAP32 iotype. It seems much cleaner to handle the
> DWAP quirk as a new port type, and move the serial_out
> accessor into the platform code. If more platforms start
> using the DWAP port, that function can be moved into a
> common location.

I posted a series[1] a couple of weeks back that does this for
DWAPB/DWAPB32 in a slightly different way which also removes the extra
interrupt handling into the platform code.

Incidentally, I've been trying to mainline a platform (picoxcell) that
has a DWAPB (with 32-bit access requirement) but struggled to get
feedback on the SoC port but hope to resubmit as a device tree based
platform.

1. http://marc.info/?l=linux-serial&m=130821602729595&w=2

> Signed-off-by: Arnd Bergmann <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: Jamie Iles <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> ---
> arch/mips/pmc-sierra/msp71xx/msp_serial.c | 32 ++++++++++-
> drivers/tty/serial/8250.c | 87 +++--------------------------
> drivers/tty/serial/8250_hub6.c | 17 ++++++

Should the hub6 stuff be in this patch?

> drivers/tty/serial/serial_core.c | 4 -
> include/linux/serial_core.h | 5 +-
> 5 files changed, 57 insertions(+), 88 deletions(-)
>
> diff --git a/arch/mips/pmc-sierra/msp71xx/msp_serial.c b/arch/mips/pmc-sierra/msp71xx/msp_serial.c
> index f726162..6c9f1d3 100644
> --- a/arch/mips/pmc-sierra/msp71xx/msp_serial.c
> +++ b/arch/mips/pmc-sierra/msp71xx/msp_serial.c
> @@ -38,6 +38,33 @@
> #include <msp_int.h>
> #include <msp_regs.h>
>
> +/* Save the LCR value so it can be re-written when a Busy Detect IRQ occurs. */
> +static inline void dwapb_save_out_value(struct uart_port *p, int offset,
> + int value)
> +{
> + struct uart_8250_port *up =
> + container_of(p, struct uart_8250_port, port);

I couldn't see that struct uart_8250_port is moved out of 8250.c, so I'm
not sure that this would work in here?

Jamie

2011-06-27 23:53:16

by David Daney

[permalink] [raw]
Subject: Re: [PATCH 3/7] serial/8250: move UPIO_TSI to powerpc

On 06/27/2011 02:45 PM, Arnd Bergmann wrote:
> This iotype is only used by the legacy_serial code in powerpc, so the
> code should live there, rather than be compiled in for every 8250
> driver.
>
> Signed-off-by: Arnd Bergmann<[email protected]>
> Cc: Benjamin Herrenschmidt<[email protected]>
> Cc: [email protected]
> Cc: Greg Kroah-Hartman<[email protected]>
> Cc: [email protected]
> ---
> arch/powerpc/kernel/legacy_serial.c | 24 ++++++++++++++++++++++++
> drivers/tty/serial/8250.c | 23 -----------------------
> 2 files changed, 24 insertions(+), 23 deletions(-)
>

This seems vaguely familiar:

https://lkml.org/lkml/2008/10/6/297

So just for the hell of it...

Acked-by: David Daney <[email protected]>

2011-06-27 23:58:59

by David Daney

[permalink] [raw]
Subject: Re: [PATCH 0/7] serial/8250: I/O accessor cleanups

On 06/27/2011 02:45 PM, Arnd Bergmann wrote:
> Hi Greg,
>
> This series of patches cleans up the part of the 8250 device
> driver that is responsible for accessing the hardware registers.
> The driver defines all sorts of methods to do that right now,
> when it really should only support memory mapped and programmed
> I/O by default, and the latter only on PC-compatible platforms
> including those that have ISA/PCMCIA/PCI buses.
>
> The series shrinks the 8250 driver by about 10% in both binary
> and source code size, hopefully with no loss of functionality,
> and it allows platforms to no longer define bogus inb/outb
> functions when they don't provide CONFIG_HAS_IOPORT. I've
> build-tested for x86 and ARM with and without HAS_IOPORT.
>
> Hopefully Ralf can provide some feedback about the three
> MIPS platforms that have code changed by this.
>
> Arnd
>
> Arnd Bergmann (7):
> serial/8250: remove obsolete RM9000 port type
> serial/8250: move alchemy I/O handler to platform code
> serial/8250: move UPIO_TSI to powerpc
> serial/8250: move DWAP support to arch/mips
> serial/8250: remove obsolete and broken PORT_RSA support
> serial/8250: sanitize fourport handling
> serial/8250: make PIO support optional
>
> arch/mips/Kconfig | 7 -
> arch/mips/alchemy/common/platform.c | 50 +++
> arch/mips/pmc-sierra/msp71xx/msp_serial.c | 32 ++-
> arch/powerpc/kernel/legacy_serial.c | 24 ++
> drivers/tty/serial/8250.c | 472 +++--------------------------
> drivers/tty/serial/8250.h | 8 +
> drivers/tty/serial/8250_hub6.c | 17 +
> drivers/tty/serial/Kconfig | 15 -
> drivers/tty/serial/serial_core.c | 4 -
> drivers/tty/serial/sunsu.c | 93 ------
> include/linux/serial.h | 2 +-
> include/linux/serial_core.h | 10 +-
> include/linux/serial_reg.h | 51 ---
> 13 files changed, 171 insertions(+), 614 deletions(-)
>
> Cc: Ralf Baechle<[email protected]>
> Cc: [email protected]
> Cc: Benjamin Herrenschmidt<[email protected]>
>


FWIW, this was basically the intention when I added the I/O accessor
functions.

If you like you can add:

Acked-by: David Daney <[email protected]>

2011-06-28 05:45:33

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 4/7] serial/8250: move DWAP support to arch/mips

On Tuesday 28 June 2011 00:15:41 Jamie Iles wrote:
> On Mon, Jun 27, 2011 at 11:45:17PM +0200, Arnd Bergmann wrote:
> > Only one board uses the UPIO_DWAP iotype, and nothing uses
> > the UPIO_DWAP32 iotype. It seems much cleaner to handle the
> > DWAP quirk as a new port type, and move the serial_out
> > accessor into the platform code. If more platforms start
> > using the DWAP port, that function can be moved into a
> > common location.
>
> I posted a series[1] a couple of weeks back that does this for
> DWAPB/DWAPB32 in a slightly different way which also removes the extra
> interrupt handling into the platform code.

Yes, your series looks better than my patch (and works!), so let's
take that instead.

> Incidentally, I've been trying to mainline a platform (picoxcell) that
> has a DWAPB (with 32-bit access requirement) but struggled to get
> feedback on the SoC port but hope to resubmit as a device tree based
> platform.

Ok. Please have a look at the xilinx zynq and the csr SiRFprimaII
submissions then. I'm planning to merge new platforms in 3.1, but
only ones that have been fully converted to the device tree and
require no board specific files. I have to admit that I mostly ignored
the picoxcell submission at the time, but I'll definitely review the
patches when you resubmit them.

> > arch/mips/pmc-sierra/msp71xx/msp_serial.c | 32 ++++++++++-
> > drivers/tty/serial/8250.c | 87 +++--------------------------
> > drivers/tty/serial/8250_hub6.c | 17 ++++++
>
> Should the hub6 stuff be in this patch?

No, I must have accidentally folded that. I'll do a new patch for that.

> > +/* Save the LCR value so it can be re-written when a Busy Detect IRQ occurs. */
> > +static inline void dwapb_save_out_value(struct uart_port *p, int offset,
> > + int value)
> > +{
> > + struct uart_8250_port *up =
> > + container_of(p, struct uart_8250_port, port);
>
> I couldn't see that struct uart_8250_port is moved out of 8250.c, so I'm
> not sure that this would work in here?

Right, I missed that as well.

Thanks!

Arnd

2011-06-28 08:09:09

by Manuel Lauss

[permalink] [raw]
Subject: Re: [PATCH 2/7] serial/8250: move alchemy I/O handler to platform code

Hi Arnd,

On Mon, Jun 27, 2011 at 11:45 PM, Arnd Bergmann <[email protected]> wrote:
> Only one platform ever sets the UPIO_AU iotype, so it's
> cleaner to define the handlers in the code that actually
> requires it, rather than building the same logic into
> every 8250 driver.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: Manuel Lauss <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> ---
> ?arch/mips/alchemy/common/platform.c | ? 50 ++++++++++++++++++++++++++++++
> ?drivers/tty/serial/8250.c ? ? ? ? ? | ? 58 -----------------------------------
> ?2 files changed, 50 insertions(+), 58 deletions(-)

This breaks serial console on all my Alchemy test system. I'll try to
figure out why.

Manuel

2011-06-28 10:03:44

by Alan

[permalink] [raw]
Subject: Re: [PATCH 7/7] serial/8250: make PIO support optional

> This makes the PIO support in the 8250 driver completely
> conditional on CONFIG_HAS_IOPORT so we can remove the bogus
> definitions from all the places that only need them for 8250.

NAK this as last time.

We had a discussion about how to fix this properly. I even posted some
sketched out patches to show how it could be addressed

Shotgunning the code with ifdefs was not that.


Get the port methods out of the core code as was proposed.

2011-06-28 10:05:51

by Alan

[permalink] [raw]
Subject: Re: [PATCH 4/7] serial/8250: move DWAP support to arch/mips

On Mon, 27 Jun 2011 23:45:17 +0200
Arnd Bergmann <[email protected]> wrote:

> Only one board uses the UPIO_DWAP iotype, and nothing uses
> the UPIO_DWAP32 iotype. It seems much cleaner to handle the
> DWAP quirk as a new port type, and move the serial_out
> accessor into the platform code. If more platforms start
> using the DWAP port, that function can be moved into a
> common location.

Half of this patch appears to be unrelated work on HUB6

Both halves look right but did you accidentally fold two patches
together ?

2011-06-28 10:09:50

by Alan

[permalink] [raw]
Subject: Re: [PATCH 6/7] serial/8250: sanitize fourport handling

On Mon, 27 Jun 2011 23:45:19 +0200
Arnd Bergmann <[email protected]> wrote:

> Support for AST fourport cards is always built into
> the 8250 driver, even if CONFIG_SERIAL_8250_FOURPORT
> is disabled. This introduces a set of macros for
> accessing the special interrupt control register on
> the fourport card, so that support is left out
> when the option is disabled, without adding more #ifdef
> lines to the driver itself.

Why not just leave it in - it seems to take almost no code and your
changes mean people setting the fourport flag are going to get strange
behaviour when it isn't compiled in.

2011-06-28 10:09:53

by Alan

[permalink] [raw]
Subject: Re: [PATCH 7/7] serial/8250: make PIO support optional


> - p->serial_in = io_serial_in;
> - p->serial_out = io_serial_out;
> + p->serial_in = mem_serial_in;
> + p->serial_out = mem_serial_out;
> break;


And this kind of stuff changing defaults is asking for trouble.

2011-06-28 10:15:28

by Alan

[permalink] [raw]
Subject: Re: [PATCH 5/7] serial/8250: remove obsolete and broken PORT_RSA support

On Mon, 27 Jun 2011 23:45:18 +0200
Arnd Bergmann <[email protected]> wrote:

> For all I can tell, the RSA support in the 8250 driver
> is not functional and has not been for a long time.

Seems fair enough

Only one that needs dealing with properly is the PIO method ifdef stuff.

2011-06-28 10:49:30

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 4/7] serial/8250: move DWAP support to arch/mips

On Tuesday 28 June 2011, Alan Cox wrote:
> On Mon, 27 Jun 2011 23:45:17 +0200
> Arnd Bergmann <[email protected]> wrote:
>
> > Only one board uses the UPIO_DWAP iotype, and nothing uses
> > the UPIO_DWAP32 iotype. It seems much cleaner to handle the
> > DWAP quirk as a new port type, and move the serial_out
> > accessor into the platform code. If more platforms start
> > using the DWAP port, that function can be moved into a
> > common location.
>
> Half of this patch appears to be unrelated work on HUB6
>
> Both halves look right but did you accidentally fold two patches
> together ?

Right, Jamie also pointed this out. When I re-send, I'll use Jamie's
version of the patch and make a proper patch for HUB6.

Arnd

2011-06-28 11:30:15

by Manuel Lauss

[permalink] [raw]
Subject: Re: [PATCH 2/7] serial/8250: move alchemy I/O handler to platform code

On Mon, Jun 27, 2011 at 11:45 PM, Arnd Bergmann <[email protected]> wrote:
> Only one platform ever sets the UPIO_AU iotype, so it's
> cleaner to define the handlers in the code that actually
> requires it, rather than building the same logic into
> every 8250 driver.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: Manuel Lauss <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> ---
> ?arch/mips/alchemy/common/platform.c | ? 50 ++++++++++++++++++++++++++++++
> ?drivers/tty/serial/8250.c ? ? ? ? ? | ? 58 -----------------------------------
> ?2 files changed, 50 insertions(+), 58 deletions(-)
>
> diff --git a/arch/mips/alchemy/common/platform.c b/arch/mips/alchemy/common/platform.c
> index 3b2c18b..750441f 100644
> --- a/arch/mips/alchemy/common/platform.c
> +++ b/arch/mips/alchemy/common/platform.c
[...]
> @@ -55,6 +103,8 @@ static void alchemy_8250_pm(struct uart_port *port, unsigned int state,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?UPF_FIXED_TYPE, ? ? ? ? ? ? ? \
> ? ? ? ? ? ? ? ?.type ? ? ? ? ? = PORT_16550A, ? ? ? ? ? ? ? ? ?\
> ? ? ? ? ? ? ? ?.pm ? ? ? ? ? ? = alchemy_8250_pm, ? ? ? ? ? ? ?\
> + ? ? ? ? ? ? ? .serial_in ? ? ?= au_serial_in, ? ? ? ? ? ? ? ? \
> + ? ? ? ? ? ? ? .serial_out ? ? = au_serial_out ? ? ? ? ? ? ? ? \
> ? ? ? ?}

This is very strange: Just this part alone (leaving 8250.c intact)
screws everything up.
The assembly for au_serial_in/out is identical in both 8250.o and
arch/mips/alchemy/common/platform.o (renamed the functions here obviously)
I have no idea what's wrong...

Manuel

2011-06-28 11:52:59

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 7/7] serial/8250: make PIO support optional

On Tuesday 28 June 2011, Alan Cox wrote:
> > This makes the PIO support in the 8250 driver completely
> > conditional on CONFIG_HAS_IOPORT so we can remove the bogus
> > definitions from all the places that only need them for 8250.
>
> NAK this as last time.
>
> We had a discussion about how to fix this properly. I even posted some
> sketched out patches to show how it could be addressed
>
> Shotgunning the code with ifdefs was not that.
>
>
> Get the port methods out of the core code as was proposed.

I got this done for all the obscure ones in patches 1-6, leaving
only the common UPIO_PORT and UPIO_MEM ones. Changing those would
mean a lot more churn, and also solving a few other problems:

Back then, I wrote:
>On Friday 11 March 2011, Alan Cox wrote:
>
>> 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.

One of the problems I encountered now is that most of the frontend drivers
(pci, of, platform, pcmcia) still require access to both MEM and PORT
methods on some platforms, but we want to use the same drivers (platform
and of) on systems that don't have port methods, so it's back to #ifdef
anyway.

Another problem is that the platform devices that set ->iotype are
usually defined in builtin code. Changing the device definitions to
set serial_in/serial_out directly means we also have to link those
functions into the kernel, even if the actual 8250 driver is a module.

A possible option might be to use ioread/iowrite to replace the
choice between MEM and PORT I/O with a common function. I thought
about that, but wasn't sure if we can alwasy call ioport_map early
enough for the console driver.

On Tuesday 28 June 2011, Alan Cox wrote:
> > - p->serial_in = io_serial_in;
> > - p->serial_out = io_serial_out;
> > + p->serial_in = mem_serial_in;
> > + p->serial_out = mem_serial_out;
> > break;
>
>And this kind of stuff changing defaults is asking for trouble.

I went back and forth between a few versions on this one, and only
later thought of one that would have been better. I did make sure
that the "default" case is only there for UPIO_PORT before and only
for UPIO_MEM after the patch. I can fix this in a better way.

The easiest approach would be to conditionalize just the inb/outb,
as below. As I explained the last time, the 8250 driver is really
the only one that has this problem because it doesn't use
ioread/iowrite and is common on both PC-style hardware and embedded
into SOCs that don't have PIO.

I would much prefer getting a build error on inb/outb for the latter
kind than having to provide bogus definitions in a lot of architectures.
For request_region, it's probably better to stub out the macro than
the users.

Arnd

--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -499,14 +499,18

static unsigned int io_serial_in(struct uart_port *p, int offset)
{
- offset = map_8250_in_reg(p, offset) << p->regshift;
+#ifdef CONFIG_HAS_IOPORT
return inb(p->iobase + offset);
+#else
+ return 0xff;
+#endif
}

static void io_serial_out(struct uart_port *p, int offset, int value)
{
- offset = map_8250_out_reg(p, offset) << p->regshift;
+#ifdef CONFIG_HAS_IOPORT
outb(value, p->iobase + offset);
+#endif
}

static void set_io_from_upio(struct uart_port *p)
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -145,8 +145,13 @@ static inline unsigned long resource_type(const struct resource *res)
}

/* Convenience shorthand with allocation */
+#ifdef CONFIG_HAS_IOPORT
#define request_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), 0)
#define request_muxed_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
+#else
+#define request_region(start,n,name) NULL
+#define request_muxed_region(start,n,name) NULL
+#endif
#define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
#define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)
#define request_mem_region_exclusive(start,n,name) \

2011-06-28 12:09:20

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 6/7] serial/8250: sanitize fourport handling

On Tuesday 28 June 2011, Alan Cox wrote:
> On Mon, 27 Jun 2011 23:45:19 +0200
> Arnd Bergmann <[email protected]> wrote:
>
> > Support for AST fourport cards is always built into
> > the 8250 driver, even if CONFIG_SERIAL_8250_FOURPORT
> > is disabled. This introduces a set of macros for
> > accessing the special interrupt control register on
> > the fourport card, so that support is left out
> > when the option is disabled, without adding more #ifdef
> > lines to the driver itself.
>
> Why not just leave it in - it seems to take almost no code and your
> changes mean people setting the fourport flag are going to get strange
> behaviour when it isn't compiled in.

Mostly to avoid the inb/outb when fourport is disabled. The regular
I/O can be handled using the indirect serial_in/out functions, but
the fourport handling requires some address calculations.

Also, now that you mention users toggling the fourport flag, I can't
see where that would ever do something good. On anything but a real
fourport card, it will lead to writing to ports you shouldn't write
to (0x1f on mmio based uarts, 18 bytes after the requested region
for regular ports).

If you have an actual AST fourport card, you won't be able to use
it unless you load the 8250_fourport driver that registers the correct
addresses, and disabling the fourport flag will then cause the
uart to stop getting interrupts.

Arnd

2011-06-28 12:21:29

by Alan

[permalink] [raw]
Subject: Re: [PATCH 7/7] serial/8250: make PIO support optional

> I got this done for all the obscure ones in patches 1-6, leaving
> only the common UPIO_PORT and UPIO_MEM ones. Changing those would
> mean a lot more churn, and also solving a few other problems:

Yes I like 1-5 ...

> I would much prefer getting a build error on inb/outb for the latter
> kind than having to provide bogus definitions in a lot of architectures.
> For request_region, it's probably better to stub out the macro than
> the users.

Agreed but that can be done by io_serial_in/io_serial_out out of the
8250.c file. Really we need p->ops or a function in another file which
provides the ioport 8250 interface and then all you'd have in the core
code would be

uart8250_set_ioport_ops(p);

which would live in 8250_ioport.c or be a "return -EINVAL' for anything
else.

That leaves request/release_std resources depending how such platforms
handle request_region attempts, and the other fourport bits which want
pushing into the 8250f_fourport driver perhaps as extra p-> ops.

if (p->startup)
p->startup(p);

etc

(Or indeed probably a p->ops-> for the lot is even saner)



Thoughts ?

2011-06-28 15:38:42

by Manuel Lauss

[permalink] [raw]
Subject: Re: [PATCH 2/7] serial/8250: move alchemy I/O handler to platform code

On Tue, Jun 28, 2011 at 1:29 PM, Manuel Lauss
<[email protected]> wrote:
> On Mon, Jun 27, 2011 at 11:45 PM, Arnd Bergmann <[email protected]> wrote:
>> Only one platform ever sets the UPIO_AU iotype, so it's
>> cleaner to define the handlers in the code that actually
>> requires it, rather than building the same logic into
>> every 8250 driver.
>>
>> Signed-off-by: Arnd Bergmann <[email protected]>
>> Cc: Ralf Baechle <[email protected]>
>> Cc: Manuel Lauss <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: [email protected]
>> ---
>> ?arch/mips/alchemy/common/platform.c | ? 50 ++++++++++++++++++++++++++++++
>> ?drivers/tty/serial/8250.c ? ? ? ? ? | ? 58 -----------------------------------
>> ?2 files changed, 50 insertions(+), 58 deletions(-)
>>
>> diff --git a/arch/mips/alchemy/common/platform.c b/arch/mips/alchemy/common/platform.c
>> index 3b2c18b..750441f 100644
>> --- a/arch/mips/alchemy/common/platform.c
>> +++ b/arch/mips/alchemy/common/platform.c
> [...]
>> @@ -55,6 +103,8 @@ static void alchemy_8250_pm(struct uart_port *port, unsigned int state,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?UPF_FIXED_TYPE, ? ? ? ? ? ? ? \
>> ? ? ? ? ? ? ? ?.type ? ? ? ? ? = PORT_16550A, ? ? ? ? ? ? ? ? ?\
>> ? ? ? ? ? ? ? ?.pm ? ? ? ? ? ? = alchemy_8250_pm, ? ? ? ? ? ? ?\
>> + ? ? ? ? ? ? ? .serial_in ? ? ?= au_serial_in, ? ? ? ? ? ? ? ? \
>> + ? ? ? ? ? ? ? .serial_out ? ? = au_serial_out ? ? ? ? ? ? ? ? \
>> ? ? ? ?}
>
> This is very strange: ?Just this part alone (leaving 8250.c intact)
> screws everything up.
> The assembly for au_serial_in/out is identical in both 8250.o and
> arch/mips/alchemy/common/platform.o (renamed the functions here obviously)
> I have no idea what's wrong...


Okay, found it. the Alchemy headers have definitions for UART_RX/TX/... with
different values; they can be deleted safely since nothing depends on them.
Here's an updated and tested patch. I took the liberty to rename the functions
a bit while at it.

diff --git a/arch/mips/alchemy/common/platform.c
b/arch/mips/alchemy/common/platform.c
index 932f697..301ccb7 100644
--- a/arch/mips/alchemy/common/platform.c
+++ b/arch/mips/alchemy/common/platform.c
@@ -16,6 +16,7 @@
#include <linux/init.h>
#include <linux/platform_device.h>
#include <linux/serial_8250.h>
+#include <linux/serial_reg.h>
#include <linux/slab.h>

#include <asm/mach-au1x00/au1xxx.h>
@@ -45,6 +46,54 @@ static void alchemy_8250_pm(struct uart_port *port,
unsigned int state,
#endif
}

+/* Au1x00 UART hardware has a weird register layout */
+static const u8 alchemy_8250_inmap[] = {
+ [UART_RX] = 0,
+ [UART_IER] = 2,
+ [UART_IIR] = 3,
+ [UART_LCR] = 5,
+ [UART_MCR] = 6,
+ [UART_LSR] = 7,
+ [UART_MSR] = 8,
+};
+
+static const u8 alchemy_8250_outmap[] = {
+ [UART_TX] = 1,
+ [UART_IER] = 2,
+ [UART_FCR] = 4,
+ [UART_LCR] = 5,
+ [UART_MCR] = 6,
+};
+
+/* sane hardware needs no mapping */
+static inline int map_8250_in_reg(struct uart_port *p, int offset)
+{
+ if (p->iotype != UPIO_AU)
+ return offset;
+ return alchemy_8250_inmap[offset];
+}
+
+static inline int map_8250_out_reg(struct uart_port *p, int offset)
+{
+ if (p->iotype != UPIO_AU)
+ return offset;
+ return alchemy_8250_outmap[offset];
+}
+
+/* sane hardware needs no mapping */
+static unsigned int alchemy_8250_in(struct uart_port *p, int offset)
+{
+ offset = map_8250_in_reg(p, offset) << p->regshift;
+ return __raw_readl(p->membase + offset);
+}
+
+static void alchemy_8250_out(struct uart_port *p, int offset, int value)
+{
+ offset = map_8250_out_reg(p, offset) << p->regshift;
+ __raw_writel(value, p->membase + offset);
+}
+
+
#define PORT(_base, _irq) \
{ \
.mapbase = _base, \
@@ -55,6 +104,8 @@ static void alchemy_8250_pm(struct uart_port *port,
unsigned int state,
UPF_FIXED_TYPE, \
.type = PORT_16550A, \
.pm = alchemy_8250_pm, \
+ .serial_in = alchemy_8250_in, \
+ .serial_out = alchemy_8250_out, \
}

static struct plat_serial8250_port au1x00_uart_data[][4] __initdata = {
diff --git a/arch/mips/include/asm/mach-au1x00/au1000.h
b/arch/mips/include/asm/mach-au1x00/au1000.h
index 610cc06..666de8e 100644
--- a/arch/mips/include/asm/mach-au1x00/au1000.h
+++ b/arch/mips/include/asm/mach-au1x00/au1000.h
@@ -1172,18 +1172,6 @@ enum soc_au1200_ints {
#define MAC_RX_BUFF3_STATUS 0x30
#define MAC_RX_BUFF3_ADDR 0x34

-#define UART_RX 0 /* Receive buffer */
-#define UART_TX 4 /* Transmit buffer */
-#define UART_IER 8 /* Interrupt Enable Register */
-#define UART_IIR 0xC /* Interrupt ID Register */
-#define UART_FCR 0x10 /* FIFO Control Register */
-#define UART_LCR 0x14 /* Line Control Register */
-#define UART_MCR 0x18 /* Modem Control Register */
-#define UART_LSR 0x1C /* Line Status Register */
-#define UART_MSR 0x20 /* Modem Status Register */
-#define UART_CLK 0x28 /* Baud Rate Clock Divider */
-#define UART_MOD_CNTRL 0x100 /* Module Control */
-
/* SSIO */
#define SSI0_STATUS 0xB1600000
# define SSI_STATUS_BF (1 << 4)
diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index cf19b26..c8f107e 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -304,50 +304,9 @@ static const struct serial8250_config uart_config[] = {
},
};

-#if defined(CONFIG_MIPS_ALCHEMY)
-
-/* Au1x00 UART hardware has a weird register layout */
-static const u8 au_io_in_map[] = {
- [UART_RX] = 0,
- [UART_IER] = 2,
- [UART_IIR] = 3,
- [UART_LCR] = 5,
- [UART_MCR] = 6,
- [UART_LSR] = 7,
- [UART_MSR] = 8,
-};
-
-static const u8 au_io_out_map[] = {
- [UART_TX] = 1,
- [UART_IER] = 2,
- [UART_FCR] = 4,
- [UART_LCR] = 5,
- [UART_MCR] = 6,
-};
-
-/* sane hardware needs no mapping */
-static inline int map_8250_in_reg(struct uart_port *p, int offset)
-{
- if (p->iotype != UPIO_AU)
- return offset;
- return au_io_in_map[offset];
-}
-
-static inline int map_8250_out_reg(struct uart_port *p, int offset)
-{
- if (p->iotype != UPIO_AU)
- return offset;
- return au_io_out_map[offset];
-}
-
-#else
-
-/* sane hardware needs no mapping */
#define map_8250_in_reg(up, offset) (offset)
#define map_8250_out_reg(up, offset) (offset)

-#endif
-
static unsigned int hub6_serial_in(struct uart_port *p, int offset)
{
offset = map_8250_in_reg(p, offset) << p->regshift;
@@ -386,18 +345,6 @@ static unsigned int mem32_serial_in(struct
uart_port *p, int offset)
return readl(p->membase + offset);
}

-static unsigned int au_serial_in(struct uart_port *p, int offset)
-{
- offset = map_8250_in_reg(p, offset) << p->regshift;
- return __raw_readl(p->membase + offset);
-}
-
-static void au_serial_out(struct uart_port *p, int offset, int value)
-{
- offset = map_8250_out_reg(p, offset) << p->regshift;
- __raw_writel(value, p->membase + offset);
-}
-
static unsigned int tsi_serial_in(struct uart_port *p, int offset)
{
unsigned int tmp;
@@ -484,11 +431,6 @@ static void set_io_from_upio(struct uart_port *p)
p->serial_out = mem32_serial_out;
break;

- case UPIO_AU:
- p->serial_in = au_serial_in;
- p->serial_out = au_serial_out;
- break;
-
case UPIO_TSI:
p->serial_in = tsi_serial_in;
p->serial_out = tsi_serial_out;

2011-06-28 17:09:30

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 2/7] serial/8250: move alchemy I/O handler to platform code

On Tuesday 28 June 2011, Manuel Lauss wrote:
> Okay, found it. the Alchemy headers have definitions for UART_RX/TX/... with
> different values; they can be deleted safely since nothing depends on them.
> Here's an updated and tested patch. I took the liberty to rename the functions
> a bit while at it.

Cool, thanks so much for tracking that down!

Please send me (in private) the Signed-off-by that you want me to add to the
patch, apparently you missed that.

I'll put your version in the series then.

Arnd