* [PATCH 1/6] Revert "serial/8250_pci: init-quirk msi support for kt serial controller"
2012-04-06 18:44 [PATCH 0/6] rework quirks for the "kt" serial port Dan Williams
@ 2012-04-06 18:49 ` Dan Williams
2012-04-06 19:05 ` Alan Cox
2012-04-06 18:49 ` [PATCH 2/6] Revert "serial/8250_pci: setup-quirk workaround for the " Dan Williams
` (5 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Dan Williams @ 2012-04-06 18:49 UTC (permalink / raw)
To: gregkh
Cc: Sudhakar Mamillapalli, linux-kernel, stable, linux-serial,
Nhan H Mai, Alan Cox
This reverts commit e86ff4a63c9fdd875ba8492577cd1ad2252f525c.
This tried to enforce the semantics of one interrupt per iir read of the
THRE (transmit-hold empty) status, but events from other sources
(particularly modem status) defeat this guarantee.
This change also broke 8250_pci suspend/resume support as
pciserial_resume_ports() re-runs .init() quirks, but does not run
.exit() quirks in pciserial_suspend_ports() leading to reports like:
sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:16.3/msi_irqs'
...and a subsequent crash. The mismatch of init/exit at suspend/resume
seems like a bug in its own right.
[stable: 3.3.x]
Cc: <stable@vger.kernel.org>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Sudhakar Mamillapalli <sudhakar@fb.com>
Reported-by: Nhan H Mai <nhan.h.mai@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/tty/serial/8250/8250_pci.c | 14 --------------
1 files changed, 0 insertions(+), 14 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index da2b0b0..1d4ccf8 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -1118,18 +1118,6 @@ pci_xr17c154_setup(struct serial_private *priv,
return pci_default_setup(priv, board, port, idx);
}
-static int try_enable_msi(struct pci_dev *dev)
-{
- /* use msi if available, but fallback to legacy otherwise */
- pci_enable_msi(dev);
- return 0;
-}
-
-static void disable_msi(struct pci_dev *dev)
-{
- pci_disable_msi(dev);
-}
-
#define PCI_VENDOR_ID_SBSMODULARIO 0x124B
#define PCI_SUBVENDOR_ID_SBSMODULARIO 0x124B
#define PCI_DEVICE_ID_OCTPRO 0x0001
@@ -1249,9 +1237,7 @@ static struct pci_serial_quirk pci_serial_quirks[] __refdata = {
.device = PCI_DEVICE_ID_INTEL_PATSBURG_KT,
.subvendor = PCI_ANY_ID,
.subdevice = PCI_ANY_ID,
- .init = try_enable_msi,
.setup = kt_serial_setup,
- .exit = disable_msi,
},
/*
* ITE
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 2/6] Revert "serial/8250_pci: setup-quirk workaround for the kt serial controller"
2012-04-06 18:44 [PATCH 0/6] rework quirks for the "kt" serial port Dan Williams
2012-04-06 18:49 ` [PATCH 1/6] Revert "serial/8250_pci: init-quirk msi support for kt serial controller" Dan Williams
@ 2012-04-06 18:49 ` Dan Williams
2012-04-06 18:49 ` [PATCH 3/6] serial/8250_pci: add a "force background timer" flag and use it for the "kt" serial port Dan Williams
` (4 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Dan Williams @ 2012-04-06 18:49 UTC (permalink / raw)
To: gregkh
Cc: Sudhakar Mamillapalli, linux-kernel, stable, linux-serial,
Nhan H Mai, Alan Cox
This reverts commit 448ac154c957c4580531fa0c8f2045816fe2f0e7.
The semantic of UPF_IIR_ONCE is only guaranteed to workaround the race
condition in the kt serial's iir register if the only source of
interrupts is THRE (fifo-empty) events. An modem status event at the
wrong time can again cause an iir read to drop the 'empty' status
leading to a hang. So, revert this in preparation for using the
existing "I don't trust my iir register" workaround in the 8250 core
(UART_BUG_THRE).
[stable: 3.3.x]
Cc: <stable@vger.kernel.org>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Sudhakar Mamillapalli <sudhakar@fb.com>
Reported-by: Nhan H Mai <nhan.h.mai@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/tty/serial/8250/8250.c | 4 +---
drivers/tty/serial/8250/8250_pci.c | 17 +----------------
include/linux/serial_core.h | 1 -
3 files changed, 2 insertions(+), 20 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
index 9b7336f..ec7d99b 100644
--- a/drivers/tty/serial/8250/8250.c
+++ b/drivers/tty/serial/8250/8250.c
@@ -1592,13 +1592,11 @@ static irqreturn_t serial8250_interrupt(int irq, void *dev_id)
do {
struct uart_8250_port *up;
struct uart_port *port;
- bool skip;
up = list_entry(l, struct uart_8250_port, list);
port = &up->port;
- skip = pass_counter && up->port.flags & UPF_IIR_ONCE;
- if (!skip && port->handle_irq(port)) {
+ if (port->handle_irq(port)) {
handled = 1;
end = NULL;
} else if (end == NULL)
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 1d4ccf8..105dcfb 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -1092,14 +1092,6 @@ static int skip_tx_en_setup(struct serial_private *priv,
return pci_default_setup(priv, board, port, idx);
}
-static int kt_serial_setup(struct serial_private *priv,
- const struct pciserial_board *board,
- struct uart_port *port, int idx)
-{
- port->flags |= UPF_IIR_ONCE;
- return skip_tx_en_setup(priv, board, port, idx);
-}
-
static int pci_eg20t_init(struct pci_dev *dev)
{
#if defined(CONFIG_SERIAL_PCH_UART) || defined(CONFIG_SERIAL_PCH_UART_MODULE)
@@ -1118,6 +1110,7 @@ pci_xr17c154_setup(struct serial_private *priv,
return pci_default_setup(priv, board, port, idx);
}
+/* This should be in linux/pci_ids.h */
#define PCI_VENDOR_ID_SBSMODULARIO 0x124B
#define PCI_SUBVENDOR_ID_SBSMODULARIO 0x124B
#define PCI_DEVICE_ID_OCTPRO 0x0001
@@ -1147,7 +1140,6 @@ pci_xr17c154_setup(struct serial_private *priv,
#define PCI_DEVICE_ID_OXSEMI_16PCI958 0x9538
#define PCIE_DEVICE_ID_NEO_2_OX_IBM 0x00F6
#define PCI_DEVICE_ID_PLX_CRONYX_OMEGA 0xc001
-#define PCI_DEVICE_ID_INTEL_PATSBURG_KT 0x1d3d
/* Unknown vendors/cards - this should not be in linux/pci_ids.h */
#define PCI_SUBDEVICE_ID_UNKNOWN_0x1584 0x1584
@@ -1232,13 +1224,6 @@ static struct pci_serial_quirk pci_serial_quirks[] __refdata = {
.subdevice = PCI_ANY_ID,
.setup = ce4100_serial_setup,
},
- {
- .vendor = PCI_VENDOR_ID_INTEL,
- .device = PCI_DEVICE_ID_INTEL_PATSBURG_KT,
- .subvendor = PCI_ANY_ID,
- .subdevice = PCI_ANY_ID,
- .setup = kt_serial_setup,
- },
/*
* ITE
*/
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index c91ace7..c56d114 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -355,7 +355,6 @@ struct uart_port {
#define UPF_CONS_FLOW ((__force upf_t) (1 << 23))
#define UPF_SHARE_IRQ ((__force upf_t) (1 << 24))
#define UPF_EXAR_EFR ((__force upf_t) (1 << 25))
-#define UPF_IIR_ONCE ((__force upf_t) (1 << 26))
/* The exact UART type is known and should not be probed. */
#define UPF_FIXED_TYPE ((__force upf_t) (1 << 27))
#define UPF_BOOT_AUTOCONF ((__force upf_t) (1 << 28))
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 3/6] serial/8250_pci: add a "force background timer" flag and use it for the "kt" serial port
2012-04-06 18:44 [PATCH 0/6] rework quirks for the "kt" serial port Dan Williams
2012-04-06 18:49 ` [PATCH 1/6] Revert "serial/8250_pci: init-quirk msi support for kt serial controller" Dan Williams
2012-04-06 18:49 ` [PATCH 2/6] Revert "serial/8250_pci: setup-quirk workaround for the " Dan Williams
@ 2012-04-06 18:49 ` Dan Williams
2012-04-06 18:49 ` [PATCH 4/6] tegra, serial8250: add ->handle_break() uart_port op Dan Williams
` (3 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Dan Williams @ 2012-04-06 18:49 UTC (permalink / raw)
To: gregkh
Cc: Sudhakar Mamillapalli, linux-kernel, stable, linux-serial,
Nhan H Mai, Alan Cox
Workaround dropped notifications in the iir register. Register reads
coincident with new interrupt notifications sometimes result in this
device clearing the interrupt event without reporting it in the read
data.
The serial core already has a heuristic for determining when a device
has an untrustworthy iir register. In this case when we apriori know
that the iir is faulty use a flag (UPF_BUG_THRE) to bypass the test and
force usage of the background timer.
Cc: Alan Cox <alan@linux.intel.com>
[stable: 3.3.x]
Cc: <stable@vger.kernel.org>
Reported-by: Nhan H Mai <nhan.h.mai@intel.com>
Reported-by: Sudhakar Mamillapalli <sudhakar@fb.com>
Tested-by: Nhan H Mai <nhan.h.mai@intel.com>
Tested-by: Sudhakar Mamillapalli <sudhakar@fb.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/tty/serial/8250/8250.c | 8 +++++---
drivers/tty/serial/8250/8250_pci.c | 17 ++++++++++++++++-
include/linux/serial_core.h | 1 +
3 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
index ec7d99b..0e2c703 100644
--- a/drivers/tty/serial/8250/8250.c
+++ b/drivers/tty/serial/8250/8250.c
@@ -2055,10 +2055,12 @@ static int serial8250_startup(struct uart_port *port)
spin_unlock_irqrestore(&up->port.lock, flags);
/*
- * If the interrupt is not reasserted, setup a timer to
- * kick the UART on a regular basis.
+ * If the interrupt is not reasserted, or we otherwise
+ * don't trust the iir, setup a timer to kick the UART
+ * on a regular basis.
*/
- if (!(iir1 & UART_IIR_NO_INT) && (iir & UART_IIR_NO_INT)) {
+ if ((!(iir1 & UART_IIR_NO_INT) && (iir & UART_IIR_NO_INT)) ||
+ up->port.flags & UPF_BUG_THRE) {
up->bugs |= UART_BUG_THRE;
pr_debug("ttyS%d - using backup timer\n",
serial_index(port));
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 105dcfb..858dca8 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -1092,6 +1092,14 @@ static int skip_tx_en_setup(struct serial_private *priv,
return pci_default_setup(priv, board, port, idx);
}
+static int kt_serial_setup(struct serial_private *priv,
+ const struct pciserial_board *board,
+ struct uart_port *port, int idx)
+{
+ port->flags |= UPF_BUG_THRE;
+ return skip_tx_en_setup(priv, board, port, idx);
+}
+
static int pci_eg20t_init(struct pci_dev *dev)
{
#if defined(CONFIG_SERIAL_PCH_UART) || defined(CONFIG_SERIAL_PCH_UART_MODULE)
@@ -1110,7 +1118,6 @@ pci_xr17c154_setup(struct serial_private *priv,
return pci_default_setup(priv, board, port, idx);
}
-/* This should be in linux/pci_ids.h */
#define PCI_VENDOR_ID_SBSMODULARIO 0x124B
#define PCI_SUBVENDOR_ID_SBSMODULARIO 0x124B
#define PCI_DEVICE_ID_OCTPRO 0x0001
@@ -1140,6 +1147,7 @@ pci_xr17c154_setup(struct serial_private *priv,
#define PCI_DEVICE_ID_OXSEMI_16PCI958 0x9538
#define PCIE_DEVICE_ID_NEO_2_OX_IBM 0x00F6
#define PCI_DEVICE_ID_PLX_CRONYX_OMEGA 0xc001
+#define PCI_DEVICE_ID_INTEL_PATSBURG_KT 0x1d3d
/* Unknown vendors/cards - this should not be in linux/pci_ids.h */
#define PCI_SUBDEVICE_ID_UNKNOWN_0x1584 0x1584
@@ -1224,6 +1232,13 @@ static struct pci_serial_quirk pci_serial_quirks[] __refdata = {
.subdevice = PCI_ANY_ID,
.setup = ce4100_serial_setup,
},
+ {
+ .vendor = PCI_VENDOR_ID_INTEL,
+ .device = PCI_DEVICE_ID_INTEL_PATSBURG_KT,
+ .subvendor = PCI_ANY_ID,
+ .subdevice = PCI_ANY_ID,
+ .setup = kt_serial_setup,
+ },
/*
* ITE
*/
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index c56d114..cbb3d12 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -355,6 +355,7 @@ struct uart_port {
#define UPF_CONS_FLOW ((__force upf_t) (1 << 23))
#define UPF_SHARE_IRQ ((__force upf_t) (1 << 24))
#define UPF_EXAR_EFR ((__force upf_t) (1 << 25))
+#define UPF_BUG_THRE ((__force upf_t) (1 << 26))
/* The exact UART type is known and should not be probed. */
#define UPF_FIXED_TYPE ((__force upf_t) (1 << 27))
#define UPF_BOOT_AUTOCONF ((__force upf_t) (1 << 28))
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 4/6] tegra, serial8250: add ->handle_break() uart_port op
2012-04-06 18:44 [PATCH 0/6] rework quirks for the "kt" serial port Dan Williams
` (2 preceding siblings ...)
2012-04-06 18:49 ` [PATCH 3/6] serial/8250_pci: add a "force background timer" flag and use it for the "kt" serial port Dan Williams
@ 2012-04-06 18:49 ` Dan Williams
2012-04-06 21:01 ` Stephen Warren
2012-04-06 18:50 ` [PATCH 5/6] serial/8250_pci: Clear FIFOs for Intel ME Serial Over Lan device on BI Dan Williams
` (2 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Dan Williams @ 2012-04-06 18:49 UTC (permalink / raw)
To: gregkh
Cc: Sudhakar Mamillapalli, Stephen Warren, linux-kernel, linux-serial,
Colin Cross, Olof Johansson, Nhan H Mai, Alan Cox, alan
The "KT" serial port has another use case for a "received break" quirk,
so before adding another special case to the 8250 core take this
opportunity to push such quirks out of the core and into a uart_port op.
Cc: Nhan H Mai <nhan.h.mai@intel.com>
Cc: Colin Cross <ccross@android.com>
Cc: Olof Johansson <olof@lixom.net>
Cc: Stephen Warren <swarren@nvidia.com>
Acked-by: Sudhakar Mamillapalli <sudhakar@fb.com>
Reported-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
arch/arm/mach-tegra/board-harmony.c | 1 +
arch/arm/mach-tegra/board-paz00.c | 2 ++
arch/arm/mach-tegra/board-seaboard.c | 1 +
arch/arm/mach-tegra/board-trimslice.c | 1 +
arch/arm/mach-tegra/devices.c | 18 +++++++++++++++++
arch/arm/mach-tegra/devices.h | 2 ++
drivers/tty/serial/8250/8250.c | 34 +++------------------------------
include/linux/serial_8250.h | 1 +
include/linux/serial_core.h | 5 +++++
9 files changed, 34 insertions(+), 31 deletions(-)
diff --git a/arch/arm/mach-tegra/board-harmony.c b/arch/arm/mach-tegra/board-harmony.c
index 789bdc9..7a4755b 100644
--- a/arch/arm/mach-tegra/board-harmony.c
+++ b/arch/arm/mach-tegra/board-harmony.c
@@ -52,6 +52,7 @@ static struct plat_serial8250_port debug_uart_platform_data[] = {
.irq = INT_UARTD,
.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_TYPE,
.type = PORT_TEGRA,
+ .handle_break = tegra_serial_handle_break,
.iotype = UPIO_MEM,
.regshift = 2,
.uartclk = 216000000,
diff --git a/arch/arm/mach-tegra/board-paz00.c b/arch/arm/mach-tegra/board-paz00.c
index 330afdf..9426e76 100644
--- a/arch/arm/mach-tegra/board-paz00.c
+++ b/arch/arm/mach-tegra/board-paz00.c
@@ -55,6 +55,7 @@ static struct plat_serial8250_port debug_uart_platform_data[] = {
.irq = INT_UARTA,
.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_TYPE,
.type = PORT_TEGRA,
+ .handle_break = tegra_serial_handle_break,
.iotype = UPIO_MEM,
.regshift = 2,
.uartclk = 216000000,
@@ -65,6 +66,7 @@ static struct plat_serial8250_port debug_uart_platform_data[] = {
.irq = INT_UARTC,
.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_TYPE,
.type = PORT_TEGRA,
+ .handle_break = tegra_serial_handle_break,
.iotype = UPIO_MEM,
.regshift = 2,
.uartclk = 216000000,
diff --git a/arch/arm/mach-tegra/board-seaboard.c b/arch/arm/mach-tegra/board-seaboard.c
index ebac65f..11b8d27 100644
--- a/arch/arm/mach-tegra/board-seaboard.c
+++ b/arch/arm/mach-tegra/board-seaboard.c
@@ -47,6 +47,7 @@ static struct plat_serial8250_port debug_uart_platform_data[] = {
/* Memory and IRQ filled in before registration */
.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_TYPE,
.type = PORT_TEGRA,
+ .handle_break = tegra_serial_handle_break,
.iotype = UPIO_MEM,
.regshift = 2,
.uartclk = 216000000,
diff --git a/arch/arm/mach-tegra/board-trimslice.c b/arch/arm/mach-tegra/board-trimslice.c
index cd52820..e7763ff 100644
--- a/arch/arm/mach-tegra/board-trimslice.c
+++ b/arch/arm/mach-tegra/board-trimslice.c
@@ -48,6 +48,7 @@ static struct plat_serial8250_port debug_uart_platform_data[] = {
.irq = INT_UARTA,
.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_TYPE,
.type = PORT_TEGRA,
+ .handle_break = tegra_serial_handle_break,
.iotype = UPIO_MEM,
.regshift = 2,
.uartclk = 216000000,
diff --git a/arch/arm/mach-tegra/devices.c b/arch/arm/mach-tegra/devices.c
index 7a2a02d..d9301bc 100644
--- a/arch/arm/mach-tegra/devices.c
+++ b/arch/arm/mach-tegra/devices.c
@@ -17,11 +17,13 @@
*/
+#include <linux/delay.h>
#include <linux/resource.h>
#include <linux/platform_device.h>
#include <linux/dma-mapping.h>
#include <linux/fsl_devices.h>
#include <linux/serial_8250.h>
+#include <linux/serial_reg.h>
#include <linux/i2c-tegra.h>
#include <linux/platform_data/tegra_usb.h>
#include <asm/pmu.h>
@@ -704,3 +706,19 @@ struct platform_device tegra_pcm_device = {
.name = "tegra-pcm-audio",
.id = -1,
};
+
+void tegra_serial_handle_break(struct uart_port *p)
+{
+ unsigned int status, tmout = 10000;
+
+ do {
+ status = p->serial_in(p, UART_LSR);
+ if (status & (UART_LSR_FIFOE | UART_LSR_BRK_ERROR_BITS))
+ status = p->serial_in(p, UART_RX);
+ else
+ break;
+ if (--tmout == 0)
+ break;
+ udelay(1);
+ } while (1);
+}
diff --git a/arch/arm/mach-tegra/devices.h b/arch/arm/mach-tegra/devices.h
index 873ecb2..c2253be 100644
--- a/arch/arm/mach-tegra/devices.h
+++ b/arch/arm/mach-tegra/devices.h
@@ -20,6 +20,7 @@
#define __MACH_TEGRA_DEVICES_H
#include <linux/platform_device.h>
+#include <linux/serial_8250.h>
extern struct platform_device tegra_gpio_device;
extern struct platform_device tegra_pinmux_device;
@@ -49,4 +50,5 @@ extern struct platform_device tegra_i2s_device2;
extern struct platform_device tegra_das_device;
extern struct platform_device tegra_pcm_device;
+void tegra_serial_handle_break(struct uart_port *p);
#endif
diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
index 0e2c703..d1e0b3d 100644
--- a/drivers/tty/serial/8250/8250.c
+++ b/drivers/tty/serial/8250/8250.c
@@ -1353,27 +1353,6 @@ static void serial8250_enable_ms(struct uart_port *port)
}
/*
- * Clear the Tegra rx fifo after a break
- *
- * FIXME: This needs to become a port specific callback once we have a
- * framework for this
- */
-static void clear_rx_fifo(struct uart_8250_port *up)
-{
- unsigned int status, tmout = 10000;
- do {
- status = serial_in(up, UART_LSR);
- if (status & (UART_LSR_FIFOE | UART_LSR_BRK_ERROR_BITS))
- status = serial_in(up, UART_RX);
- else
- break;
- if (--tmout == 0)
- break;
- udelay(1);
- } while (1);
-}
-
-/*
* serial8250_rx_chars: processes according to the passed in LSR
* value, and returns the remaining LSR bits not handled
* by this Rx routine.
@@ -1406,20 +1385,10 @@ serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr)
up->lsr_saved_flags = 0;
if (unlikely(lsr & UART_LSR_BRK_ERROR_BITS)) {
- /*
- * For statistics only
- */
if (lsr & UART_LSR_BI) {
lsr &= ~(UART_LSR_FE | UART_LSR_PE);
up->port.icount.brk++;
/*
- * If tegra port then clear the rx fifo to
- * accept another break/character.
- */
- if (up->port.type == PORT_TEGRA)
- clear_rx_fifo(up);
-
- /*
* We do the SysRQ and SAK checking
* here because otherwise the break
* may get masked by ignore_status_mask
@@ -3047,6 +3016,7 @@ static int __devinit serial8250_probe(struct platform_device *dev)
port.serial_in = p->serial_in;
port.serial_out = p->serial_out;
port.handle_irq = p->handle_irq;
+ port.handle_break = p->handle_break;
port.set_termios = p->set_termios;
port.pm = p->pm;
port.dev = &dev->dev;
@@ -3219,6 +3189,8 @@ int serial8250_register_port(struct uart_port *port)
uart->port.set_termios = port->set_termios;
if (port->pm)
uart->port.pm = port->pm;
+ if (port->handle_break)
+ uart->port.handle_break = port->handle_break;
if (serial8250_isa_config != NULL)
serial8250_isa_config(0, &uart->port,
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 8f012f8..a522fd9 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -38,6 +38,7 @@ struct plat_serial8250_port {
int (*handle_irq)(struct uart_port *);
void (*pm)(struct uart_port *, unsigned int state,
unsigned old);
+ void (*handle_break)(struct uart_port *);
};
/*
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index cbb3d12..82f6147 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -308,6 +308,7 @@ struct uart_port {
int (*handle_irq)(struct uart_port *);
void (*pm)(struct uart_port *, unsigned int state,
unsigned int old);
+ void (*handle_break)(struct uart_port *);
unsigned int irq; /* irq number */
unsigned long irqflags; /* irq flags */
unsigned int uartclk; /* base uart clock */
@@ -521,6 +522,10 @@ uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
static inline int uart_handle_break(struct uart_port *port)
{
struct uart_state *state = port->state;
+
+ if (port->handle_break)
+ port->handle_break(port);
+
#ifdef SUPPORT_SYSRQ
if (port->cons && port->cons->index == port->line) {
if (!port->sysrq) {
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 4/6] tegra, serial8250: add ->handle_break() uart_port op
2012-04-06 18:49 ` [PATCH 4/6] tegra, serial8250: add ->handle_break() uart_port op Dan Williams
@ 2012-04-06 21:01 ` Stephen Warren
2012-04-06 21:28 ` Williams, Dan J
0 siblings, 1 reply; 14+ messages in thread
From: Stephen Warren @ 2012-04-06 21:01 UTC (permalink / raw)
To: Dan Williams
Cc: gregkh, Sudhakar Mamillapalli, linux-kernel, linux-serial,
Colin Cross, Olof Johansson, Nhan H Mai, Alan Cox, alan
On 04/06/2012 12:49 PM, Dan Williams wrote:
> The "KT" serial port has another use case for a "received break" quirk,
> so before adding another special case to the 8250 core take this
> opportunity to push such quirks out of the core and into a uart_port op.
This doesn't seem quite right. Why do the board files have to set up
this .handle_break function; they're already setting .type=PORT_TEGRA,
which should be enough to drive the setup of any required quirks.
If plat_serial8250_port must contain this field, then
drivers/tty/serial/of_serial.c needs a similar change so that this all
works when booting using device tree.
I'm not sure what the implication is of moving the call to clr_fifo()
into uart_handle_break(). What's the benefit of one location over the other?
If the callback function is to no longer live in 8250.c itself,
arch/arm/mach-tegra/devices.c isn't logically a good place to put it,
and that file will be going away once we get rid of all the board files
and move solely to device tree.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/6] tegra, serial8250: add ->handle_break() uart_port op
2012-04-06 21:01 ` Stephen Warren
@ 2012-04-06 21:28 ` Williams, Dan J
2012-04-06 21:56 ` Stephen Warren
0 siblings, 1 reply; 14+ messages in thread
From: Williams, Dan J @ 2012-04-06 21:28 UTC (permalink / raw)
To: Stephen Warren
Cc: gregkh, Sudhakar Mamillapalli, linux-kernel, linux-serial,
Colin Cross, Olof Johansson, Nhan H Mai, Alan Cox, alan
On Fri, Apr 6, 2012 at 2:01 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/06/2012 12:49 PM, Dan Williams wrote:
>> The "KT" serial port has another use case for a "received break" quirk,
>> so before adding another special case to the 8250 core take this
>> opportunity to push such quirks out of the core and into a uart_port op.
>
> This doesn't seem quite right. Why do the board files have to set up
> this .handle_break function; they're already setting .type=PORT_TEGRA,
> which should be enough to drive the setup of any required quirks.
Because struct serial8250_config does not convey any uart_port ops.
> If plat_serial8250_port must contain this field, then
> drivers/tty/serial/of_serial.c needs a similar change so that this all
> works when booting using device tree.
>
> I'm not sure what the implication is of moving the call to clr_fifo()
> into uart_handle_break(). What's the benefit of one location over the other?
This was the location where the core was already doing it's break
handling, so it made sense to check here if the device had any quirks
to run. There shouldn't be any implications because the core was
already doing clear_rx_fifo() immediately before calling
uart_handle_break. Here is the relevant hunk with a bit more context:
@@ -1399,34 +1378,24 @@ serial8250_rx_chars(struct uart_8250_port *up,
unsigned char lsr)
*/
ch = 0;
flag = TTY_NORMAL;
up->port.icount.rx++;
lsr |= up->lsr_saved_flags;
up->lsr_saved_flags = 0;
if (unlikely(lsr & UART_LSR_BRK_ERROR_BITS)) {
- /*
- * For statistics only
- */
if (lsr & UART_LSR_BI) {
lsr &= ~(UART_LSR_FE | UART_LSR_PE);
up->port.icount.brk++;
/*
- * If tegra port then clear the rx fifo to
- * accept another break/character.
- */
- if (up->port.type == PORT_TEGRA)
- clear_rx_fifo(up);
-
- /*
* We do the SysRQ and SAK checking
* here because otherwise the break
* may get masked by ignore_status_mask
* or read_status_mask.
*/
if (uart_handle_break(&up->port))
goto ignore_char;
} else if (lsr & UART_LSR_PE)
up->port.icount.parity++;
else if (lsr & UART_LSR_FE)
> If the callback function is to no longer live in 8250.c itself,
> arch/arm/mach-tegra/devices.c isn't logically a good place to put it,
> and that file will be going away once we get rid of all the board files
> and move solely to device tree.
Can you help me with an incremental patch to fix this up? I can
muddle my way through Tegra internals, but I already missed the
of_serial.c hook.
--
Dan
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 4/6] tegra, serial8250: add ->handle_break() uart_port op
2012-04-06 21:28 ` Williams, Dan J
@ 2012-04-06 21:56 ` Stephen Warren
2012-04-06 22:25 ` Williams, Dan J
0 siblings, 1 reply; 14+ messages in thread
From: Stephen Warren @ 2012-04-06 21:56 UTC (permalink / raw)
To: Williams, Dan J
Cc: gregkh, Sudhakar Mamillapalli, linux-kernel, linux-serial,
Colin Cross, Olof Johansson, Nhan H Mai, Alan Cox, alan
On 04/06/2012 03:28 PM, Williams, Dan J wrote:
> On Fri, Apr 6, 2012 at 2:01 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 04/06/2012 12:49 PM, Dan Williams wrote:
>>> The "KT" serial port has another use case for a "received break" quirk,
>>> so before adding another special case to the 8250 core take this
>>> opportunity to push such quirks out of the core and into a uart_port op.
>>
>> This doesn't seem quite right. Why do the board files have to set up
>> this .handle_break function; they're already setting .type=PORT_TEGRA,
>> which should be enough to drive the setup of any required quirks.
>
> Because struct serial8250_config does not convey any uart_port ops.
But couldn't it be enhanced to do so, just like this patch added a field
to struct uart_port for this? If you went this route, then the change
would be entirely isolated within 8250.c, so you could drop all the
arch/arm/mach-tegra changes, and also not need to update of_serial.c.
>> I'm not sure what the implication is of moving the call to clr_fifo()
>> into uart_handle_break(). What's the benefit of one location over the other?
>
> This was the location where the core was already doing it's break
> handling, so it made sense to check here if the device had any quirks
> to run. There shouldn't be any implications because the core was
> already doing clear_rx_fifo() immediately before calling
> uart_handle_break. Here is the relevant hunk with a bit more context:
Ah OK, that part seems fine then.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/6] tegra, serial8250: add ->handle_break() uart_port op
2012-04-06 21:56 ` Stephen Warren
@ 2012-04-06 22:25 ` Williams, Dan J
0 siblings, 0 replies; 14+ messages in thread
From: Williams, Dan J @ 2012-04-06 22:25 UTC (permalink / raw)
To: Stephen Warren
Cc: gregkh, Sudhakar Mamillapalli, linux-kernel, linux-serial,
Colin Cross, Olof Johansson, Nhan H Mai, Alan Cox, alan
On Fri, Apr 6, 2012 at 2:56 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/06/2012 03:28 PM, Williams, Dan J wrote:
>> On Fri, Apr 6, 2012 at 2:01 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 04/06/2012 12:49 PM, Dan Williams wrote:
>>>> The "KT" serial port has another use case for a "received break" quirk,
>>>> so before adding another special case to the 8250 core take this
>>>> opportunity to push such quirks out of the core and into a uart_port op.
>>>
>>> This doesn't seem quite right. Why do the board files have to set up
>>> this .handle_break function; they're already setting .type=PORT_TEGRA,
>>> which should be enough to drive the setup of any required quirks.
>>
>> Because struct serial8250_config does not convey any uart_port ops.
>
> But couldn't it be enhanced to do so, just like this patch added a field
> to struct uart_port for this? If you went this route, then the change
> would be entirely isolated within 8250.c, so you could drop all the
> arch/arm/mach-tegra changes, and also not need to update of_serial.c.
That's kind of the point, it seems the goal of serial port hardware
design is to find new and interesting ways to make them fail. Putting
everyone's fail in 8250.c is messy so when we tried to add another
"handle_break" quirk we were rightly asked to make the core interface
generic and push the logic out of the core.
I don't see how moving quirk routines into serial8250_config makes
this any cleaner, we'd either be left with two methods for specifying
quirks, or requiring custom serial8250_configs for drivers that only
want to set one of the ops.
Hmm, is there any precendent for an openfirmware serial port that
needs one of these custom ops?
unsigned int (*serial_in)(struct uart_port *, int);
void (*serial_out)(struct uart_port *, int, int);
void (*set_termios)(struct uart_port *,
struct ktermios *new,
struct ktermios *old);
int (*handle_irq)(struct uart_port *);
void (*pm)(struct uart_port *, unsigned int state,
unsigned int old);
void (*handle_break)(struct uart_port *);
...I would be surprised if this is the first time this has come up
given the prevalence of broken uarts.
--
Dan
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 5/6] serial/8250_pci: Clear FIFOs for Intel ME Serial Over Lan device on BI
2012-04-06 18:44 [PATCH 0/6] rework quirks for the "kt" serial port Dan Williams
` (3 preceding siblings ...)
2012-04-06 18:49 ` [PATCH 4/6] tegra, serial8250: add ->handle_break() uart_port op Dan Williams
@ 2012-04-06 18:50 ` Dan Williams
2012-04-06 18:50 ` [RFC PATCH 6/6] serial/8250_pci: fix suspend/resume vs init/exit quirks Dan Williams
2012-04-06 19:14 ` [PATCH 0/6] rework quirks for the "kt" serial port Greg KH
6 siblings, 0 replies; 14+ messages in thread
From: Dan Williams @ 2012-04-06 18:50 UTC (permalink / raw)
To: gregkh
Cc: Sudhakar Mamillapalli, Nhan H Mai, linux-kernel, linux-serial,
Alan Cox
From: Sudhakar Mamillapalli <sudhakar@fb.com>
When using Serial Over Lan (SOL) over the virtual serial port in a Intel
management engine (ME) device, on device reset the serial FIFOs need to
be cleared to keep the FIFO indexes in-sync between the host and the
engine.
On a reset the serial device assertes BI, so using that as a cue FIFOs
are cleared. So for this purpose a new handle_break callback has been
added. One other problem is that the serial registers might temporarily
go to 0 on reset of this device. So instead of using the IER register
read, if 0 returned use the ier value in uart_8250_port. This is hidden
under a custom serial_in.
Cc: Alan Cox <alan@linux.intel.com>
Cc: Nhan H Mai <nhan.h.mai@intel.com>
Signed-off-by: Sudhakar Mamillapalli <sudhakar@fb.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/tty/serial/8250/8250.c | 10 +++++++++
drivers/tty/serial/8250/8250.h | 2 ++
drivers/tty/serial/8250/8250_pci.c | 39 ++++++++++++++++++++++++++++++++++++
3 files changed, 51 insertions(+), 0 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
index d1e0b3d..3fc7619 100644
--- a/drivers/tty/serial/8250/8250.c
+++ b/drivers/tty/serial/8250/8250.c
@@ -590,6 +590,16 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
}
}
+void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p)
+{
+ unsigned char fcr;
+
+ serial8250_clear_fifos(p);
+ fcr = uart_config[p->port.type].fcr;
+ serial_out(p, UART_FCR, fcr);
+}
+EXPORT_SYMBOL_GPL(serial8250_clear_and_reinit_fifos);
+
/*
* IER sleep support. UARTs which have EFRs need the "extended
* capability" bit enabled. Note that on XR16C850s, we need to
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index ae027be..5efa6c5 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -86,6 +86,8 @@ struct serial8250_config {
#define SERIAL8250_SHARE_IRQS 0
#endif
+void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p);
+
#if defined(__alpha__) && !defined(CONFIG_PCI)
/*
* Digital did something really horribly wrong with the OUT1 and OUT2
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 858dca8..024551a 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -17,6 +17,7 @@
#include <linux/slab.h>
#include <linux/delay.h>
#include <linux/tty.h>
+#include <linux/serial_reg.h>
#include <linux/serial_core.h>
#include <linux/8250_pci.h>
#include <linux/bitops.h>
@@ -1092,11 +1093,49 @@ static int skip_tx_en_setup(struct serial_private *priv,
return pci_default_setup(priv, board, port, idx);
}
+static void kt_handle_break(struct uart_port *p)
+{
+ struct uart_8250_port *up =
+ container_of(p, struct uart_8250_port, port);
+ /*
+ * On receipt of a BI, serial device in Intel ME (Intel
+ * management engine) needs to have its fifos cleared for sane
+ * SOL (Serial Over Lan) output.
+ */
+ serial8250_clear_and_reinit_fifos(up);
+}
+
+static unsigned int kt_serial_in(struct uart_port *p, int offset)
+{
+ struct uart_8250_port *up =
+ container_of(p, struct uart_8250_port, port);
+ unsigned int val;
+
+ /*
+ * When the Intel ME (management engine) gets reset its serial
+ * port registers could return 0 momentarily. Functions like
+ * serial8250_console_write, read and save the IER, perform
+ * some operation and then restore it. In order to avoid
+ * setting IER register inadvertently to 0, if the value read
+ * is 0, double check with ier value in uart_8250_port and use
+ * that instead. up->ier should be the same value as what is
+ * currently configured.
+ */
+ val = inb(p->iobase + offset);
+ if (offset == UART_IER) {
+ if (val == 0)
+ val = up->ier;
+ }
+ return val;
+}
+
static int kt_serial_setup(struct serial_private *priv,
const struct pciserial_board *board,
struct uart_port *port, int idx)
{
port->flags |= UPF_BUG_THRE;
+ port->serial_in = kt_serial_in;
+ port->handle_break = kt_handle_break;
return skip_tx_en_setup(priv, board, port, idx);
}
^ permalink raw reply related [flat|nested] 14+ messages in thread* [RFC PATCH 6/6] serial/8250_pci: fix suspend/resume vs init/exit quirks
2012-04-06 18:44 [PATCH 0/6] rework quirks for the "kt" serial port Dan Williams
` (4 preceding siblings ...)
2012-04-06 18:50 ` [PATCH 5/6] serial/8250_pci: Clear FIFOs for Intel ME Serial Over Lan device on BI Dan Williams
@ 2012-04-06 18:50 ` Dan Williams
2012-04-06 19:14 ` [PATCH 0/6] rework quirks for the "kt" serial port Greg KH
6 siblings, 0 replies; 14+ messages in thread
From: Dan Williams @ 2012-04-06 18:50 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, linux-serial, Alan Cox
Commit e86ff4a6 "serial/8250_pci: init-quirk msi support for kt serial
controller" introduced a regression in suspend/resume by causing msi's
to be enabled twice without an intervening disable.
That patch has since been reverted, but by inspection it seems that
pciserial_suspend_ports() should be invoking .exit() quirks to release
resources acquired during .init().
Cc: Alan Cox <alan@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/tty/serial/8250/8250_pci.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 024551a..24ea98c 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -2814,6 +2814,12 @@ void pciserial_suspend_ports(struct serial_private *priv)
for (i = 0; i < priv->nr; i++)
if (priv->line[i] >= 0)
serial8250_suspend_port(priv->line[i]);
+
+ /*
+ * Ensure that every init quirk is properly torn down
+ */
+ if (priv->quirk->exit)
+ priv->quirk->exit(priv->dev);
}
EXPORT_SYMBOL_GPL(pciserial_suspend_ports);
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 0/6] rework quirks for the "kt" serial port
2012-04-06 18:44 [PATCH 0/6] rework quirks for the "kt" serial port Dan Williams
` (5 preceding siblings ...)
2012-04-06 18:50 ` [RFC PATCH 6/6] serial/8250_pci: fix suspend/resume vs init/exit quirks Dan Williams
@ 2012-04-06 19:14 ` Greg KH
2012-04-06 19:49 ` Williams, Dan J
6 siblings, 1 reply; 14+ messages in thread
From: Greg KH @ 2012-04-06 19:14 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-kernel, linux-serial, alan
On Fri, Apr 06, 2012 at 11:44:22AM -0700, Dan Williams wrote:
> The kt serial port intermittently reports unreliable data in its IIR
> register and needs to reset fifos to stay in sync with firmware when the
> device is reset.
>
> The IIR fixes that went into 3.3 were not complete so patch 3 replaces
> them with an explicit way to turn on UART_BUG_THRE (i.e. the "I don't
> trust my iir" flag).
>
So the first 3 patches should go into 3.4 (and 3.3-stable) kernels,
right?
> A new fix for resetting the fifos on device reset is patch 5. Alan
> asked to make this a new quirk rather than add more workarounds to the
> core (patch 4).
These can wait for 3.5?
> Patch 6 is an untested RFC / RFT from anyone that has a serial
> suspend/resume use case with an 'init' quirk that has resources that
> need de-allocation at 'exit'.
And I'll hold off on this one entirely for now, right?
greg k-h
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 0/6] rework quirks for the "kt" serial port
2012-04-06 19:14 ` [PATCH 0/6] rework quirks for the "kt" serial port Greg KH
@ 2012-04-06 19:49 ` Williams, Dan J
0 siblings, 0 replies; 14+ messages in thread
From: Williams, Dan J @ 2012-04-06 19:49 UTC (permalink / raw)
To: Greg KH; +Cc: linux-kernel, linux-serial, alan
On Fri, Apr 6, 2012 at 12:14 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Apr 06, 2012 at 11:44:22AM -0700, Dan Williams wrote:
>> The kt serial port intermittently reports unreliable data in its IIR
>> register and needs to reset fifos to stay in sync with firmware when the
>> device is reset.
>>
>> The IIR fixes that went into 3.3 were not complete so patch 3 replaces
>> them with an explicit way to turn on UART_BUG_THRE (i.e. the "I don't
>> trust my iir" flag).
>>
>
> So the first 3 patches should go into 3.4 (and 3.3-stable) kernels,
> right?
Yes, since this fixes the workaround that went into 3.3 properly and
addresses a regression.
>> A new fix for resetting the fifos on device reset is patch 5. Alan
>> asked to make this a new quirk rather than add more workarounds to the
>> core (patch 4).
>
> These can wait for 3.5?
I think they can wait especially because of the cross tree changes.
>> Patch 6 is an untested RFC / RFT from anyone that has a serial
>> suspend/resume use case with an 'init' quirk that has resources that
>> need de-allocation at 'exit'.
>
> And I'll hold off on this one entirely for now, right?
Yeah, if anyone runs into a similar problem with serial suspend /
resume they may want to take a look at this patch. But for now I
don't have a valid test case for it.
--
Dan
^ permalink raw reply [flat|nested] 14+ messages in thread