pci1xxxx is a PCIe switch with a multi-function endpoint on one of its
downstream ports. Quad-uart is one of the functions in the multi-function
endpoint. This patch adds device driver for the quad-uart function and
enumerates between 1 to 4 instances of uarts based on the PCIe subsystem
device ID.
The changes from v1->v2->v3->v4->v5 are mentioned in each patch in the
patchset.
Thanks to Andy Shevchenko, Ilpo Jarvinen, Chritophe JAILLET, Geert
Uytterhoeven, Greg KH for their review comments.
Kumaravel Thiagarajan (4):
8250: microchip: pci1xxxx: Add driver for quad-uart support.
8250: microchip: pci1xxxx: Add serial8250_pci_setup_port definition in
8250_pcilib.c
8250: microchip: pci1xxxx: Add RS485 support to quad-uart driver
8250: microchip: pci1xxxx: Add power management functions to quad-uart
driver
MAINTAINERS | 7 +
drivers/tty/serial/8250/8250_pci.c | 24 +-
drivers/tty/serial/8250/8250_pci1xxxx.c | 544 ++++++++++++++++++++++++
drivers/tty/serial/8250/8250_pcilib.c | 38 ++
drivers/tty/serial/8250/8250_pcilib.h | 9 +
drivers/tty/serial/8250/8250_port.c | 8 +
drivers/tty/serial/8250/Kconfig | 15 +
drivers/tty/serial/8250/Makefile | 2 +
include/uapi/linux/serial_core.h | 3 +
9 files changed, 628 insertions(+), 22 deletions(-)
create mode 100644 drivers/tty/serial/8250/8250_pci1xxxx.c
create mode 100644 drivers/tty/serial/8250/8250_pcilib.c
create mode 100644 drivers/tty/serial/8250/8250_pcilib.h
--
2.25.1
pci1xxxx's quad-uart function has the capability to wake up UART
from suspend state. Enable wakeup before entering into suspend and
disable wakeup on resume.
Co-developed-by: Tharun Kumar P <[email protected]>
Signed-off-by: Tharun Kumar P <[email protected]>
Signed-off-by: Kumaravel Thiagarajan <[email protected]>
---
Changes in v5:
- Corrected commit message
Changes in v4:
- No Change
Changes in v3:
- Handled race condition in suspend and resume callbacks
Changes in v2:
- Use DEFINE_SIMPLE_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS.
- Use pm_sleep_ptr instead of CONFIG_PM_SLEEP.
- Change the return data type of pci1xxxx_port_suspend to bool from int.
---
drivers/tty/serial/8250/8250_pci1xxxx.c | 116 ++++++++++++++++++++++++
1 file changed, 116 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
index bead9fd4019e..fa3477a7ea59 100644
--- a/drivers/tty/serial/8250/8250_pci1xxxx.c
+++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
@@ -187,6 +187,116 @@ static const struct serial_rs485 pci1xxxx_rs485_supported = {
/* Delay RTS before send is not supported */
};
+static bool pci1xxxx_port_suspend(int line)
+{
+ struct uart_8250_port *up = serial8250_get_port(line);
+ struct uart_port *port = &up->port;
+ struct tty_port *tport = &port->state->port;
+ unsigned long flags;
+ bool ret = false;
+ u8 wakeup_mask;
+
+ mutex_lock(&tport->mutex);
+ if (port->suspended == 0 && port->dev) {
+ wakeup_mask = readb(up->port.membase + UART_WAKE_MASK_REG);
+
+ spin_lock_irqsave(&port->lock, flags);
+ port->mctrl &= ~TIOCM_OUT2;
+ port->ops->set_mctrl(port, port->mctrl);
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ ret = (wakeup_mask & UART_WAKE_SRCS) != UART_WAKE_SRCS;
+ }
+
+ writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
+ mutex_unlock(&tport->mutex);
+
+ return ret;
+}
+
+static void pci1xxxx_port_resume(int line)
+{
+ struct uart_8250_port *up = serial8250_get_port(line);
+ struct uart_port *port = &up->port;
+ struct tty_port *tport = &port->state->port;
+ unsigned long flags;
+
+ mutex_lock(&tport->mutex);
+ writeb(UART_BLOCK_SET_ACTIVE, port->membase + UART_ACTV_REG);
+ writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
+
+ if (port->suspended == 0) {
+ spin_lock_irqsave(&port->lock, flags);
+ port->mctrl |= TIOCM_OUT2;
+ port->ops->set_mctrl(port, port->mctrl);
+ spin_unlock_irqrestore(&port->lock, flags);
+ }
+ mutex_unlock(&tport->mutex);
+}
+
+static int pci1xxxx_suspend(struct device *dev)
+{
+ struct pci1xxxx_8250 *priv = dev_get_drvdata(dev);
+ struct pci_dev *pcidev = to_pci_dev(dev);
+ bool wakeup = false;
+ unsigned int data;
+ void __iomem *p;
+ int i;
+
+ for (i = 0; i < priv->nr; i++) {
+ if (priv->line[i] >= 0) {
+ serial8250_suspend_port(priv->line[i]);
+ wakeup |= pci1xxxx_port_suspend(priv->line[i]);
+ }
+ }
+
+ p = pci_ioremap_bar(pcidev, 0);
+ if (!p) {
+ dev_err(dev, "remapping of bar 0 memory failed");
+ return -ENOMEM;
+ }
+
+ data = readl(p + UART_RESET_REG);
+ writel(data | UART_RESET_D3_RESET_DISABLE, p + UART_RESET_REG);
+
+ if (wakeup)
+ writeb(UART_PCI_CTRL_D3_CLK_ENABLE, p + UART_PCI_CTRL_REG);
+
+ iounmap(p);
+ device_set_wakeup_enable(dev, true);
+ pci_wake_from_d3(pcidev, true);
+
+ return 0;
+}
+
+static int pci1xxxx_resume(struct device *dev)
+{
+ struct pci1xxxx_8250 *priv = dev_get_drvdata(dev);
+ struct pci_dev *pcidev = to_pci_dev(dev);
+ unsigned int data;
+ void __iomem *p;
+ int i;
+
+ p = pci_ioremap_bar(pcidev, 0);
+ if (!p) {
+ dev_err(dev, "remapping of bar 0 memory failed");
+ return -ENOMEM;
+ }
+
+ data = readl(p + UART_RESET_REG);
+ writel(data & ~UART_RESET_D3_RESET_DISABLE, p + UART_RESET_REG);
+ iounmap(p);
+
+ for (i = 0; i < priv->nr; i++) {
+ if (priv->line[i] >= 0) {
+ pci1xxxx_port_resume(priv->line[i]);
+ serial8250_resume_port(priv->line[i]);
+ }
+ }
+
+ return 0;
+}
+
static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
struct uart_8250_port *port, int idx)
{
@@ -404,6 +514,9 @@ static void pci1xxxx_serial_remove(struct pci_dev *dev)
}
}
+static DEFINE_SIMPLE_DEV_PM_OPS(pci1xxxx_pm_ops, pci1xxxx_suspend,
+ pci1xxxx_resume);
+
static const struct pci_device_id pci1xxxx_pci_tbl[] = {
{ PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11010) },
{ PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11101) },
@@ -418,6 +531,9 @@ static struct pci_driver pci1xxxx_pci_driver = {
.name = "pci1xxxx serial",
.probe = pci1xxxx_serial_probe,
.remove = pci1xxxx_serial_remove,
+ .driver = {
+ .pm = pm_sleep_ptr(&pci1xxxx_pm_ops),
+ },
.id_table = pci1xxxx_pci_tbl,
};
module_pci_driver(pci1xxxx_pci_driver);
--
2.25.1
pci1xxxx is a PCIe switch with a multi-function endpoint on one of
its downstream ports. Quad-uart is one of the functions in the
multi-function endpoint. This driver loads for the quad-uart and
enumerates single or multiple instances of uart based on the PCIe
subsystem device ID.
Co-developed-by: Tharun Kumar P <[email protected]>
Signed-off-by: Tharun Kumar P <[email protected]>
Signed-off-by: Kumaravel Thiagarajan <[email protected]>
---
Changes in v5:
- Used tabs instead of spaces in MACRO definitions for readability
- Removed assignments that are not required
- Removed redundant blank lines
Changes in v4:
- Renamed pci_setup_port to serial8250_pci_setup_port
- Added Copyright information to 8250_pcilib.c
Changes in v3:
- Used NSEC_PER_SEC, HZ_PER_MHZ, FIELD_PREP, FIELD_GET MACROs wherever
necessary
- Handled failure case of serial8250_register_8250_port properly
- Moved pci_setup_port to 8250_pcilib.c
Changes in v2:
- Use only the 62.5 MHz for baud clock.
- Define custom implementation for get_divisor and set_divisor.
- Use BOTHER instead of UPF_SPD_CUST for non standard baud rates
(untested).
- Correct indentation in clock divisor computation.
- Remove unnecessary call to pci_save_state in probe function.
- Fix null pointer dereference in probe function.
- Move pci1xxxx_rs485_config to a separate patch.
- Depends on SERIAL_8250_PCI & default to SERIAL_8250.
- Change PORT_MCHP16550A to 100 from 124.
---
MAINTAINERS | 7 +
drivers/tty/serial/8250/8250_pci1xxxx.c | 372 ++++++++++++++++++++++++
drivers/tty/serial/8250/8250_port.c | 8 +
drivers/tty/serial/8250/Kconfig | 10 +
drivers/tty/serial/8250/Makefile | 1 +
include/uapi/linux/serial_core.h | 3 +
6 files changed, 401 insertions(+)
create mode 100644 drivers/tty/serial/8250/8250_pci1xxxx.c
diff --git a/MAINTAINERS b/MAINTAINERS
index d30f26e07cd3..aa98deaba249 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13434,6 +13434,13 @@ F: Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml
F: drivers/nvmem/microchip-otpc.c
F: include/dt-bindings/nvmem/microchip,sama7g5-otpc.h
+MICROCHIP PCIe UART DRIVER
+M: Kumaravel Thiagarajan <[email protected]>
+M: Tharun Kumar P <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/tty/serial/8250/8250_pci1xxxx.c
+
MICROCHIP PWM DRIVER
M: Claudiu Beznea <[email protected]>
L: [email protected] (moderated for non-subscribers)
diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
new file mode 100644
index 000000000000..9dd7aca76e58
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
@@ -0,0 +1,372 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Probe module for 8250/16550-type MCHP PCI serial ports.
+ *
+ * Based on drivers/tty/serial/8250/8250_pci.c,
+ *
+ * Copyright (C) 2022 Microchip Technology Inc., All Rights Reserved.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/serial_core.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/units.h>
+#include <linux/tty.h>
+
+#include <asm/byteorder.h>
+
+#include "8250.h"
+
+#define PCI_DEVICE_ID_EFAR_PCI12000 0xa002
+#define PCI_DEVICE_ID_EFAR_PCI11010 0xa012
+#define PCI_DEVICE_ID_EFAR_PCI11101 0xa022
+#define PCI_DEVICE_ID_EFAR_PCI11400 0xa032
+#define PCI_DEVICE_ID_EFAR_PCI11414 0xa042
+
+#define PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_4p 0x0001
+#define PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p012 0x0002
+#define PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013 0x0003
+#define PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023 0x0004
+#define PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123 0x0005
+#define PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p01 0x0006
+#define PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02 0x0007
+#define PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03 0x0008
+#define PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12 0x0009
+#define PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13 0x000a
+#define PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23 0x000b
+#define PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p0 0x000c
+#define PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1 0x000d
+#define PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2 0x000e
+#define PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3 0x000f
+
+#define PCI_SUBDEVICE_ID_EFAR_PCI12000 0xa002
+#define PCI_SUBDEVICE_ID_EFAR_PCI11010 0xa012
+#define PCI_SUBDEVICE_ID_EFAR_PCI11101 0xa022
+#define PCI_SUBDEVICE_ID_EFAR_PCI11400 0xa032
+#define PCI_SUBDEVICE_ID_EFAR_PCI11414 0xa042
+
+#define UART_ACTV_REG 0x11
+#define UART_BLOCK_SET_ACTIVE BIT(0)
+
+#define UART_PCI_CTRL_REG 0x80
+#define UART_PCI_CTRL_SET_MULTIPLE_MSI BIT(4)
+#define UART_PCI_CTRL_D3_CLK_ENABLE BIT(0)
+
+#define ADCL_CFG_REG 0x40
+#define ADCL_CFG_POL_SEL BIT(2)
+#define ADCL_CFG_PIN_SEL BIT(1)
+#define ADCL_CFG_EN BIT(0)
+
+#define UART_BIT_SAMPLE_CNT 16
+#define BAUD_CLOCK_DIV_INT_MSK GENMASK(31, 8)
+#define ADCL_CFG_RTS_DELAY_MASK GENMASK(11, 8)
+#define UART_CLOCK_DEFAULT (62.5 * HZ_PER_MHZ)
+
+#define UART_WAKE_REG 0x8C
+#define UART_WAKE_MASK_REG 0x90
+#define UART_WAKE_N_PIN BIT(2)
+#define UART_WAKE_NCTS BIT(1)
+#define UART_WAKE_INT BIT(0)
+#define UART_WAKE_SRCS \
+ (UART_WAKE_N_PIN | UART_WAKE_NCTS | UART_WAKE_INT)
+
+#define UART_BAUD_CLK_DIVISOR_REG 0x54
+
+#define UART_RESET_REG 0x94
+#define UART_RESET_D3_RESET_DISABLE BIT(16)
+
+struct pci1xxxx_8250 {
+ struct pci_dev *dev;
+ unsigned int nr;
+ void __iomem *membase;
+ int line[];
+};
+
+static int pci1xxxx_get_num_ports(struct pci_dev *dev)
+{
+ switch (dev->subsystem_device) {
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p0:
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2:
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3:
+ default:
+ return 1;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p01:
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02:
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03:
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12:
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23:
+ return 2;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p012:
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123:
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013:
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023:
+ return 3;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_4p:
+ case PCI_SUBDEVICE_ID_EFAR_PCI11414:
+ return 4;
+ }
+}
+
+static unsigned int pci1xxxx_get_divisor(struct uart_port *port,
+ unsigned int baud, unsigned int *frac)
+{
+ unsigned int quot;
+
+ /*
+ * Calculate baud rate sampling period in nanoseconds.
+ * Fractional part x denotes x/255 parts of a nanosecond.
+ */
+ quot = (NSEC_PER_SEC / (baud * UART_BIT_SAMPLE_CNT));
+ *frac = (((NSEC_PER_SEC - (quot * baud * UART_BIT_SAMPLE_CNT)) /
+ UART_BIT_SAMPLE_CNT) * 255) / baud;
+
+ return quot;
+}
+
+static void pci1xxxx_set_divisor(struct uart_port *port, unsigned int baud,
+ unsigned int quot, unsigned int frac)
+{
+ writel(FIELD_PREP(BAUD_CLOCK_DIV_INT_MSK, quot) | frac,
+ port->membase + UART_BAUD_CLK_DIVISOR_REG);
+}
+
+static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
+ struct uart_8250_port *port, int idx)
+{
+ int first_offset;
+ int offset;
+
+ switch (priv->dev->subsystem_device) {
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12:
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123:
+ first_offset = 256;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2:
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23:
+ first_offset = 512;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3:
+ first_offset = 768;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
+ first_offset = 256;
+ break;
+ default:
+ first_offset = 0;
+ break;
+ }
+
+ switch (priv->dev->subsystem_device) {
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02:
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023:
+ if (idx > 0)
+ idx++;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03:
+ if (idx > 0)
+ idx += 2;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
+ if (idx > 0)
+ idx++;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013:
+ if (idx > 1)
+ idx++;
+ break;
+ }
+
+ offset = first_offset + idx * 256;
+ port->port.flags |= UPF_FIXED_TYPE | UPF_SKIP_TEST;
+ port->port.type = PORT_MCHP16550A;
+ port->port.set_termios = serial8250_do_set_termios;
+ port->port.get_divisor = pci1xxxx_get_divisor;
+ port->port.set_divisor = pci1xxxx_set_divisor;
+ writeb(UART_BLOCK_SET_ACTIVE, port->port.membase + UART_ACTV_REG);
+ writeb(UART_WAKE_SRCS, port->port.membase + UART_WAKE_REG);
+ writeb(UART_WAKE_N_PIN, port->port.membase + UART_WAKE_MASK_REG);
+
+ return 0;
+}
+
+static void pci1xxxx_irq_assign(struct pci1xxxx_8250 *priv,
+ struct uart_8250_port *uart, int idx)
+{
+ int irq_idx;
+
+ switch (priv->dev->subsystem_device) {
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p0:
+ case PCI_SUBDEVICE_ID_EFAR_PCI12000:
+ case PCI_SUBDEVICE_ID_EFAR_PCI11010:
+ case PCI_SUBDEVICE_ID_EFAR_PCI11101:
+ case PCI_SUBDEVICE_ID_EFAR_PCI11400:
+ default:
+ irq_idx = 0;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
+ irq_idx = 1;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2:
+ irq_idx = 2;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3:
+ irq_idx = 3;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p01:
+ irq_idx = idx;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02:
+ if (idx > 0)
+ idx++;
+ irq_idx = idx;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03:
+ if (idx > 0)
+ idx += 2;
+ irq_idx = idx;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12:
+ irq_idx = idx + 1;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
+ if (idx > 0)
+ idx += 1;
+ irq_idx = idx + 1;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23:
+ irq_idx = idx + 2;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p012:
+ irq_idx = idx;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013:
+ if (idx > 1)
+ idx++;
+ irq_idx = idx;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023:
+ if (idx > 0)
+ idx++;
+ irq_idx = idx;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123:
+ irq_idx = idx + 1;
+ break;
+ case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_4p:
+ case PCI_SUBDEVICE_ID_EFAR_PCI11414:
+ irq_idx = idx;
+ break;
+ }
+ uart->port.irq = pci_irq_vector(priv->dev, irq_idx);
+}
+
+static int pci1xxxx_serial_probe(struct pci_dev *pdev,
+ const struct pci_device_id *id)
+{
+ unsigned int nr_ports, i;
+ struct pci1xxxx_8250 *priv;
+ struct uart_8250_port uart;
+ struct device *dev;
+ int num_vectors;
+ int rc;
+
+ dev = &pdev->dev;
+ rc = pcim_enable_device(pdev);
+ if (rc)
+ return rc;
+
+ nr_ports = pci1xxxx_get_num_ports(pdev);
+
+ priv = devm_kzalloc(dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->membase = pcim_iomap(pdev, 0, 0);
+ priv->dev = pdev;
+ priv->nr = nr_ports;
+ pci_set_master(pdev);
+
+ num_vectors = pci_alloc_irq_vectors(pdev, 1, 4, PCI_IRQ_ALL_TYPES);
+ if (num_vectors < 0)
+ return num_vectors;
+
+ memset(&uart, 0, sizeof(uart));
+ uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_PORT;
+ uart.port.uartclk = UART_CLOCK_DEFAULT;
+ uart.port.dev = dev;
+
+ if (num_vectors == 4)
+ writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI,
+ priv->membase + UART_PCI_CTRL_REG);
+ else
+ uart.port.irq = pci_irq_vector(pdev, 0);
+
+ for (i = 0; i < nr_ports; i++)
+ priv->line[i] = -ENOSPC;
+
+ for (i = 0; i < nr_ports; i++) {
+ if (num_vectors == 4)
+ pci1xxxx_irq_assign(priv, &uart, i);
+
+ rc = pci1xxxx_setup(priv, &uart, i);
+ if (rc) {
+ dev_warn(dev, "Failed to setup port %u\n", i);
+ break;
+ }
+ priv->line[i] = serial8250_register_8250_port(&uart);
+ if (priv->line[i] < 0) {
+ dev_err(dev,
+ "Couldn't register serial port %lx, irq %d, type %d, error %d\n",
+ uart.port.iobase, uart.port.irq,
+ uart.port.iotype, priv->line[i]);
+ break;
+ }
+ }
+
+ pci_set_drvdata(pdev, priv);
+
+ return 0;
+}
+
+static void pci1xxxx_serial_remove(struct pci_dev *dev)
+{
+ struct pci1xxxx_8250 *priv = pci_get_drvdata(dev);
+ int i;
+
+ for (i = 0; i < priv->nr; i++) {
+ if (priv->line[i] >= 0)
+ serial8250_unregister_port(priv->line[i]);
+ }
+}
+
+static const struct pci_device_id pci1xxxx_pci_tbl[] = {
+ { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11010) },
+ { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11101) },
+ { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11400) },
+ { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11414) },
+ { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI12000) },
+ {}
+};
+MODULE_DEVICE_TABLE(pci, pci1xxxx_pci_tbl);
+
+static struct pci_driver pci1xxxx_pci_driver = {
+ .name = "pci1xxxx serial",
+ .probe = pci1xxxx_serial_probe,
+ .remove = pci1xxxx_serial_remove,
+ .id_table = pci1xxxx_pci_tbl,
+};
+module_pci_driver(pci1xxxx_pci_driver);
+
+MODULE_DESCRIPTION("Microchip Technology Inc. PCIe to UART module");
+MODULE_AUTHOR("Kumaravel Thiagarajan <[email protected]>");
+MODULE_AUTHOR("Tharun Kumar P <[email protected]>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 1d2a43214b48..ec2fe5fd7b02 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -313,6 +313,14 @@ static const struct serial8250_config uart_config[] = {
.rxtrig_bytes = {1, 4, 8, 14},
.flags = UART_CAP_FIFO,
},
+ [PORT_MCHP16550A] = {
+ .name = "MCHP16550A",
+ .fifo_size = 256,
+ .tx_loadsz = 256,
+ .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01,
+ .rxtrig_bytes = {2, 66, 130, 194},
+ .flags = UART_CAP_FIFO,
+ },
};
/* Uart divisor latch read */
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index d0b49e15fbf5..1c41722d8ac5 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -291,6 +291,16 @@ config SERIAL_8250_HUB6
To compile this driver as a module, choose M here: the module
will be called 8250_hub6.
+config SERIAL_8250_PCI1XXXX
+ tristate "Microchip 8250 based serial port"
+ depends on SERIAL_8250_PCI
+ default SERIAL_8250
+ help
+ Select this option if you have a setup with Microchip PCIe
+ Switch with serial port enabled and wish to enable 8250
+ serial driver for the serial interface. This driver support
+ will ensure to support baud rates upto 1.5Mpbs.
+
#
# Misc. options/drivers.
#
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index bee908f99ea0..fbc7d47c25bd 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_SERIAL_8250_ACCENT) += 8250_accent.o
obj-$(CONFIG_SERIAL_8250_BOCA) += 8250_boca.o
obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554) += 8250_exar_st16c554.o
obj-$(CONFIG_SERIAL_8250_HUB6) += 8250_hub6.o
+obj-$(CONFIG_SERIAL_8250_PCI1XXXX) += 8250_pci1xxxx.o
obj-$(CONFIG_SERIAL_8250_FSL) += 8250_fsl.o
obj-$(CONFIG_SERIAL_8250_MEN_MCB) += 8250_men_mcb.o
obj-$(CONFIG_SERIAL_8250_DW) += 8250_dw.o
diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index 3ba34d8378bd..281fa286555c 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -207,6 +207,9 @@
/* Atheros AR933X SoC */
#define PORT_AR933X 99
+/* MCHP 16550A UART with 256 byte FIFOs */
+#define PORT_MCHP16550A 100
+
/* ARC (Synopsys) on-chip UART */
#define PORT_ARC 101
--
2.25.1
Move implementation of setup_port API to serial8250_pci_setup_port
Co-developed-by: Tharun Kumar P <[email protected]>
Signed-off-by: Tharun Kumar P <[email protected]>
Signed-off-by: Kumaravel Thiagarajan <[email protected]>
---
Changes in v5:
- This is the new patch added in v5 version of this patchset
- Moved implementation of setup_port from 8250_pci.c to 8250_pcilib.c
---
drivers/tty/serial/8250/8250_pci.c | 24 ++--------------
drivers/tty/serial/8250/8250_pci1xxxx.c | 6 ++++
drivers/tty/serial/8250/8250_pcilib.c | 38 +++++++++++++++++++++++++
drivers/tty/serial/8250/8250_pcilib.h | 9 ++++++
drivers/tty/serial/8250/Kconfig | 5 ++++
drivers/tty/serial/8250/Makefile | 1 +
6 files changed, 61 insertions(+), 22 deletions(-)
create mode 100644 drivers/tty/serial/8250/8250_pcilib.c
create mode 100644 drivers/tty/serial/8250/8250_pcilib.h
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 6f66dc2ebacc..69ff367b08de 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -24,6 +24,7 @@
#include <asm/io.h>
#include "8250.h"
+#include "8250_pcilib.h"
/*
* init function returns:
@@ -89,28 +90,7 @@ static int
setup_port(struct serial_private *priv, struct uart_8250_port *port,
u8 bar, unsigned int offset, int regshift)
{
- struct pci_dev *dev = priv->dev;
-
- if (bar >= PCI_STD_NUM_BARS)
- return -EINVAL;
-
- if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
- if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
- return -ENOMEM;
-
- port->port.iotype = UPIO_MEM;
- port->port.iobase = 0;
- port->port.mapbase = pci_resource_start(dev, bar) + offset;
- port->port.membase = pcim_iomap_table(dev)[bar] + offset;
- port->port.regshift = regshift;
- } else {
- port->port.iotype = UPIO_PORT;
- port->port.iobase = pci_resource_start(dev, bar) + offset;
- port->port.mapbase = 0;
- port->port.membase = NULL;
- port->port.regshift = 0;
- }
- return 0;
+ return serial8250_pci_setup_port(priv->dev, port, bar, offset, regshift);
}
/*
diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
index 9dd7aca76e58..02b9c6959dcc 100644
--- a/drivers/tty/serial/8250/8250_pci1xxxx.c
+++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
@@ -22,6 +22,7 @@
#include <asm/byteorder.h>
#include "8250.h"
+#include "8250_pcilib.h"
#define PCI_DEVICE_ID_EFAR_PCI12000 0xa002
#define PCI_DEVICE_ID_EFAR_PCI11010 0xa012
@@ -143,6 +144,7 @@ static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
{
int first_offset;
int offset;
+ int ret;
switch (priv->dev->subsystem_device) {
case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
@@ -191,6 +193,10 @@ static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
port->port.set_termios = serial8250_do_set_termios;
port->port.get_divisor = pci1xxxx_get_divisor;
port->port.set_divisor = pci1xxxx_set_divisor;
+ ret = serial8250_pci_setup_port(priv->dev, port, 0, offset, 0);
+ if (ret < 0)
+ return ret;
+
writeb(UART_BLOCK_SET_ACTIVE, port->port.membase + UART_ACTV_REG);
writeb(UART_WAKE_SRCS, port->port.membase + UART_WAKE_REG);
writeb(UART_WAKE_N_PIN, port->port.membase + UART_WAKE_MASK_REG);
diff --git a/drivers/tty/serial/8250/8250_pcilib.c b/drivers/tty/serial/8250/8250_pcilib.c
new file mode 100644
index 000000000000..e5a4a9b22c81
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_pcilib.c
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * 8250 PCI library.
+ *
+ * Copyright (C) 2001 Russell King, All Rights Reserved.
+ */
+#include <linux/errno.h>
+#include <linux/ioport.h>
+#include <linux/pci.h>
+#include <linux/types.h>
+
+#include "8250.h"
+
+int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port,
+ u8 bar, unsigned int offset, int regshift)
+{
+ if (bar >= PCI_STD_NUM_BARS)
+ return -EINVAL;
+
+ if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
+ if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
+ return -ENOMEM;
+
+ port->port.iotype = UPIO_MEM;
+ port->port.iobase = 0;
+ port->port.mapbase = pci_resource_start(dev, bar) + offset;
+ port->port.membase = pcim_iomap_table(dev)[bar] + offset;
+ port->port.regshift = regshift;
+ } else {
+ port->port.iotype = UPIO_PORT;
+ port->port.iobase = pci_resource_start(dev, bar) + offset;
+ port->port.mapbase = 0;
+ port->port.membase = NULL;
+ port->port.regshift = 0;
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(serial8250_pci_setup_port);
diff --git a/drivers/tty/serial/8250/8250_pcilib.h b/drivers/tty/serial/8250/8250_pcilib.h
new file mode 100644
index 000000000000..41ef01d5c3c5
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_pcilib.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * 8250 PCI library header file.
+ *
+ * Copyright (C) 2001 Russell King, All Rights Reserved.
+ */
+
+int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port, u8 bar,
+ unsigned int offset, int regshift);
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 1c41722d8ac5..f67542470eae 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -132,6 +132,7 @@ config SERIAL_8250_DMA
config SERIAL_8250_PCI
tristate "8250/16550 PCI device support"
depends on SERIAL_8250 && PCI
+ select SERIAL_8250_PCILIB
default SERIAL_8250
help
This builds standard PCI serial support. You may be able to
@@ -294,6 +295,7 @@ config SERIAL_8250_HUB6
config SERIAL_8250_PCI1XXXX
tristate "Microchip 8250 based serial port"
depends on SERIAL_8250_PCI
+ select SERIAL_8250_PCILIB
default SERIAL_8250
help
Select this option if you have a setup with Microchip PCIe
@@ -510,6 +512,9 @@ config SERIAL_8250_MID
Intel Medfield SOC and various other Intel platforms that is not
covered by the more generic SERIAL_8250_PCI option.
+config SERIAL_8250_PCILIB
+ bool
+
config SERIAL_8250_PERICOM
tristate "Support for Pericom and Acces I/O serial ports"
default SERIAL_8250
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index fbc7d47c25bd..98202fdf39f8 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_SERIAL_8250) += 8250.o 8250_base.o
8250_base-$(CONFIG_SERIAL_8250_DMA) += 8250_dma.o
8250_base-$(CONFIG_SERIAL_8250_DWLIB) += 8250_dwlib.o
8250_base-$(CONFIG_SERIAL_8250_FINTEK) += 8250_fintek.o
+8250_base-$(CONFIG_SERIAL_8250_PCILIB) += 8250_pcilib.o
obj-$(CONFIG_SERIAL_8250_GSC) += 8250_gsc.o
obj-$(CONFIG_SERIAL_8250_PCI) += 8250_pci.o
obj-$(CONFIG_SERIAL_8250_EXAR) += 8250_exar.o
--
2.25.1
On Thu, Nov 17, 2022 at 10:31:24AM +0530, Kumaravel Thiagarajan wrote:
> Move implementation of setup_port API to serial8250_pci_setup_port
Don't you have a dependency issue here?
The subject also wrong. This should be 'serial: 8250_pci: ...'.
And for your code something like 'serial: 8250_pci1xxxx: ...'
in the rest of the series.
--
With Best Regards,
Andy Shevchenko
On Thu, Nov 17, 2022 at 10:31:23AM +0530, Kumaravel Thiagarajan wrote:
> pci1xxxx is a PCIe switch with a multi-function endpoint on one of
> its downstream ports. Quad-uart is one of the functions in the
> multi-function endpoint. This driver loads for the quad-uart and
> enumerates single or multiple instances of uart based on the PCIe
> subsystem device ID.
Getting better!
...
> +struct pci1xxxx_8250 {
> + struct pci_dev *dev;
Call it pdev to distinguish with regular struct device.
> + unsigned int nr;
> + void __iomem *membase;
> + int line[];
> +};
...
> +static int pci1xxxx_get_num_ports(struct pci_dev *dev)
> +{
> + switch (dev->subsystem_device) {
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p0:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3:
> + default:
You can even start with a default, so it will be more visible.
But the way it's now is also okay.
> + return 1;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p01:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23:
> + return 2;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p012:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023:
> + return 3;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_4p:
> + case PCI_SUBDEVICE_ID_EFAR_PCI11414:
> + return 4;
> + }
> +}
...
> + quot = (NSEC_PER_SEC / (baud * UART_BIT_SAMPLE_CNT));
Too many parentheses.
> + *frac = (((NSEC_PER_SEC - (quot * baud * UART_BIT_SAMPLE_CNT)) /
Ditto.
> + UART_BIT_SAMPLE_CNT) * 255) / baud;
...
> + switch (priv->dev->subsystem_device) {
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123:
> + first_offset = 256;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23:
> + first_offset = 512;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3:
> + first_offset = 768;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> + first_offset = 256;
> + break;
Can't it be moved to the above list?
> + default:
> + first_offset = 0;
> + break;
> + }
...
> + switch (priv->dev->subsystem_device) {
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023:
> + if (idx > 0)
> + idx++;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03:
> + if (idx > 0)
> + idx += 2;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> + if (idx > 0)
> + idx++;
> + break;
Can it be moved to the above list?
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013:
> + if (idx > 1)
> + idx++;
> + break;
default?
> + }
...
> + switch (priv->dev->subsystem_device) {
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p0:
> + case PCI_SUBDEVICE_ID_EFAR_PCI12000:
> + case PCI_SUBDEVICE_ID_EFAR_PCI11010:
> + case PCI_SUBDEVICE_ID_EFAR_PCI11101:
> + case PCI_SUBDEVICE_ID_EFAR_PCI11400:
> + default:
> + irq_idx = 0;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
> + irq_idx = 1;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2:
> + irq_idx = 2;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3:
> + irq_idx = 3;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p01:
> + irq_idx = idx;
This line is duplicated. I told you how to avoid duplication.
Use -1 outside of the switch-case and check that after.
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02:
> + if (idx > 0)
> + idx++;
> + irq_idx = idx;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03:
> + if (idx > 0)
> + idx += 2;
> + irq_idx = idx;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12:
> + irq_idx = idx + 1;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> + if (idx > 0)
> + idx += 1;
> + irq_idx = idx + 1;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23:
> + irq_idx = idx + 2;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p012:
> + irq_idx = idx;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013:
> + if (idx > 1)
> + idx++;
> + irq_idx = idx;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023:
> + if (idx > 0)
> + idx++;
> + irq_idx = idx;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123:
> + irq_idx = idx + 1;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_4p:
> + case PCI_SUBDEVICE_ID_EFAR_PCI11414:
> + irq_idx = idx;
> + break;
Try to make this entire switch-case more compact. It's possible.
> + }
...
> + dev = &pdev->dev;
You can do it in the definition block above, since we know that dev is always
valid at this point.
...
> + priv->membase = pcim_iomap(pdev, 0, 0);
Never fails?
...
> + for (i = 0; i < nr_ports; i++) {
> + if (num_vectors == 4)
> + pci1xxxx_irq_assign(priv, &uart, i);
> +
> + rc = pci1xxxx_setup(priv, &uart, i);
> + if (rc) {
> + dev_warn(dev, "Failed to setup port %u\n", i);
> + break;
If it's not a fatal error, why break? Don't you need to continue for the rest?
Otherwise use
return dev_err_probe(...);
pattern.
> + }
> + priv->line[i] = serial8250_register_8250_port(&uart);
> + if (priv->line[i] < 0) {
> + dev_err(dev,
> + "Couldn't register serial port %lx, irq %d, type %d, error %d\n",
> + uart.port.iobase, uart.port.irq,
> + uart.port.iotype, priv->line[i]);
> + break;
Ditto.
> + }
> + }
--
With Best Regards,
Andy Shevchenko
On Thu, 17 Nov 2022, Kumaravel Thiagarajan wrote:
> Move implementation of setup_port API to serial8250_pci_setup_port
>
> Co-developed-by: Tharun Kumar P <[email protected]>
> Signed-off-by: Tharun Kumar P <[email protected]>
> Signed-off-by: Kumaravel Thiagarajan <[email protected]>
> ---
> Changes in v5:
> - This is the new patch added in v5 version of this patchset
> - Moved implementation of setup_port from 8250_pci.c to 8250_pcilib.c
>
> ---
> drivers/tty/serial/8250/8250_pci.c | 24 ++--------------
> drivers/tty/serial/8250/8250_pci1xxxx.c | 6 ++++
> drivers/tty/serial/8250/8250_pcilib.c | 38 +++++++++++++++++++++++++
> drivers/tty/serial/8250/8250_pcilib.h | 9 ++++++
> drivers/tty/serial/8250/Kconfig | 5 ++++
> drivers/tty/serial/8250/Makefile | 1 +
> 6 files changed, 61 insertions(+), 22 deletions(-)
> create mode 100644 drivers/tty/serial/8250/8250_pcilib.c
> create mode 100644 drivers/tty/serial/8250/8250_pcilib.h
>
> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> index 6f66dc2ebacc..69ff367b08de 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -24,6 +24,7 @@
> #include <asm/io.h>
>
> #include "8250.h"
> +#include "8250_pcilib.h"
>
> /*
> * init function returns:
> @@ -89,28 +90,7 @@ static int
> setup_port(struct serial_private *priv, struct uart_8250_port *port,
> u8 bar, unsigned int offset, int regshift)
> {
> - struct pci_dev *dev = priv->dev;
> -
> - if (bar >= PCI_STD_NUM_BARS)
> - return -EINVAL;
> -
> - if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
> - if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
> - return -ENOMEM;
> -
> - port->port.iotype = UPIO_MEM;
> - port->port.iobase = 0;
> - port->port.mapbase = pci_resource_start(dev, bar) + offset;
> - port->port.membase = pcim_iomap_table(dev)[bar] + offset;
> - port->port.regshift = regshift;
> - } else {
> - port->port.iotype = UPIO_PORT;
> - port->port.iobase = pci_resource_start(dev, bar) + offset;
> - port->port.mapbase = 0;
> - port->port.membase = NULL;
> - port->port.regshift = 0;
> - }
> - return 0;
> + return serial8250_pci_setup_port(priv->dev, port, bar, offset, regshift);
> }
>
> /*
> diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
> index 9dd7aca76e58..02b9c6959dcc 100644
> --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> @@ -22,6 +22,7 @@
> #include <asm/byteorder.h>
>
> #include "8250.h"
> +#include "8250_pcilib.h"
>
> #define PCI_DEVICE_ID_EFAR_PCI12000 0xa002
> #define PCI_DEVICE_ID_EFAR_PCI11010 0xa012
> @@ -143,6 +144,7 @@ static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
> {
> int first_offset;
> int offset;
> + int ret;
>
> switch (priv->dev->subsystem_device) {
> case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
> @@ -191,6 +193,10 @@ static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
> port->port.set_termios = serial8250_do_set_termios;
> port->port.get_divisor = pci1xxxx_get_divisor;
> port->port.set_divisor = pci1xxxx_set_divisor;
> + ret = serial8250_pci_setup_port(priv->dev, port, 0, offset, 0);
> + if (ret < 0)
> + return ret;
> +
> writeb(UART_BLOCK_SET_ACTIVE, port->port.membase + UART_ACTV_REG);
> writeb(UART_WAKE_SRCS, port->port.membase + UART_WAKE_REG);
> writeb(UART_WAKE_N_PIN, port->port.membase + UART_WAKE_MASK_REG);
> diff --git a/drivers/tty/serial/8250/8250_pcilib.c b/drivers/tty/serial/8250/8250_pcilib.c
> new file mode 100644
> index 000000000000..e5a4a9b22c81
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_pcilib.c
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * 8250 PCI library.
> + *
> + * Copyright (C) 2001 Russell King, All Rights Reserved.
> + */
> +#include <linux/errno.h>
> +#include <linux/ioport.h>
> +#include <linux/pci.h>
> +#include <linux/types.h>
> +
> +#include "8250.h"
> +
> +int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port,
> + u8 bar, unsigned int offset, int regshift)
> +{
> + if (bar >= PCI_STD_NUM_BARS)
> + return -EINVAL;
> +
> + if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
> + if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
> + return -ENOMEM;
> +
> + port->port.iotype = UPIO_MEM;
> + port->port.iobase = 0;
> + port->port.mapbase = pci_resource_start(dev, bar) + offset;
> + port->port.membase = pcim_iomap_table(dev)[bar] + offset;
> + port->port.regshift = regshift;
> + } else {
> + port->port.iotype = UPIO_PORT;
> + port->port.iobase = pci_resource_start(dev, bar) + offset;
> + port->port.mapbase = 0;
> + port->port.membase = NULL;
> + port->port.regshift = 0;
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(serial8250_pci_setup_port);
> diff --git a/drivers/tty/serial/8250/8250_pcilib.h b/drivers/tty/serial/8250/8250_pcilib.h
> new file mode 100644
> index 000000000000..41ef01d5c3c5
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_pcilib.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * 8250 PCI library header file.
> + *
> + * Copyright (C) 2001 Russell King, All Rights Reserved.
> + */
> +
> +int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port, u8 bar,
> + unsigned int offset, int regshift);
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index 1c41722d8ac5..f67542470eae 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -132,6 +132,7 @@ config SERIAL_8250_DMA
> config SERIAL_8250_PCI
> tristate "8250/16550 PCI device support"
> depends on SERIAL_8250 && PCI
> + select SERIAL_8250_PCILIB
> default SERIAL_8250
> help
> This builds standard PCI serial support. You may be able to
> @@ -294,6 +295,7 @@ config SERIAL_8250_HUB6
> config SERIAL_8250_PCI1XXXX
> tristate "Microchip 8250 based serial port"
> depends on SERIAL_8250_PCI
> + select SERIAL_8250_PCILIB
> default SERIAL_8250
> help
> Select this option if you have a setup with Microchip PCIe
> @@ -510,6 +512,9 @@ config SERIAL_8250_MID
> Intel Medfield SOC and various other Intel platforms that is not
> covered by the more generic SERIAL_8250_PCI option.
>
> +config SERIAL_8250_PCILIB
> + bool
> +
> config SERIAL_8250_PERICOM
> tristate "Support for Pericom and Acces I/O serial ports"
> default SERIAL_8250
> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index fbc7d47c25bd..98202fdf39f8 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_SERIAL_8250) += 8250.o 8250_base.o
> 8250_base-$(CONFIG_SERIAL_8250_DMA) += 8250_dma.o
> 8250_base-$(CONFIG_SERIAL_8250_DWLIB) += 8250_dwlib.o
> 8250_base-$(CONFIG_SERIAL_8250_FINTEK) += 8250_fintek.o
> +8250_base-$(CONFIG_SERIAL_8250_PCILIB) += 8250_pcilib.o
> obj-$(CONFIG_SERIAL_8250_GSC) += 8250_gsc.o
> obj-$(CONFIG_SERIAL_8250_PCI) += 8250_pci.o
> obj-$(CONFIG_SERIAL_8250_EXAR) += 8250_exar.o
You should swap patches 1/4 and 2/4 order so that the pcilib is added
first so that you can use it directly when adding your driver.
--
i.
> Sent: Thursday, November 17, 2022 12:26 PM
> To: Kumaravel Thiagarajan - I21417
> <[email protected]>
> Subject: Re: [PATCH v5 tty-next 2/4] 8250: microchip: pci1xxxx: Add
> serial8250_pci_setup_port definition in 8250_pcilib.c
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Thu, Nov 17, 2022 at 10:31:24AM +0530, Kumaravel Thiagarajan wrote:
> > Move implementation of setup_port API to serial8250_pci_setup_port
>
> Don't you have a dependency issue here?
Okay, I will explain the need for the changes done in commit description.
Thanks,
Tharun Kumar P
> From: Andy Shevchenko <[email protected]>
> Sent: Thursday, November 17, 2022 1:55 PM
> To: Kumaravel Thiagarajan - I21417
> <[email protected]>
> Subject: Re: [PATCH v5 tty-next 1/4] 8250: microchip: pci1xxxx: Add driver for
> quad-uart support.
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> > + idx++;
> > + break;
>
> default?
Do you suggest using default case even if nothing needs to be done there?
Thanks,
Tharun Kumar P
On Fri, Nov 18, 2022 at 09:37:02AM +0000, [email protected] wrote:
> > From: Andy Shevchenko <[email protected]>
> > Sent: Thursday, November 17, 2022 1:55 PM
...
> > default?
>
> Do you suggest using default case even if nothing needs to be done there?
Depends on the how critical the default case is. If nothing should be done and
it's okay, then
default:
break;
is what I meant. Otherwise you need to put something else instead of 'break;'
statement.
--
With Best Regards,
Andy Shevchenko
On Fri, Nov 18, 2022 at 09:40:37AM +0000, [email protected] wrote:
> > Sent: Thursday, November 17, 2022 12:26 PM
> > To: Kumaravel Thiagarajan - I21417
> > <[email protected]>
> > Subject: Re: [PATCH v5 tty-next 2/4] 8250: microchip: pci1xxxx: Add
> > serial8250_pci_setup_port definition in 8250_pcilib.c
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > On Thu, Nov 17, 2022 at 10:31:24AM +0530, Kumaravel Thiagarajan wrote:
> > > Move implementation of setup_port API to serial8250_pci_setup_port
> >
> > Don't you have a dependency issue here?
>
> Okay, I will explain the need for the changes done in commit description.
What I meant is that the 8250_pci patch should be _prerequisite_ to your stuff
and not otherwise.
--
With Best Regards,
Andy Shevchenko
> From: Andy Shevchenko <[email protected]>
> Sent: Friday, November 18, 2022 4:14 PM
> To: Tharunkumar Pasumarthi - I67821
> <[email protected]>
> Subject: Re: [PATCH v5 tty-next 2/4] 8250: microchip: pci1xxxx: Add
> serial8250_pci_setup_port definition in 8250_pcilib.c
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> > > Don't you have a dependency issue here?
> >
> > Okay, I will explain the need for the changes done in commit description.
>
> What I meant is that the 8250_pci patch should be _prerequisite_ to your
> stuff and not otherwise.
Hi Andy,
So, do you suggest having these changes done as first patch of the patchset prior to patches
specific for our driver?
Thanks,
Tharun Kumar P
On Sat, Nov 19, 2022 at 03:50:02AM +0000, [email protected] wrote:
> > From: Andy Shevchenko <[email protected]>
> > Sent: Friday, November 18, 2022 4:14 PM
> > To: Tharunkumar Pasumarthi - I67821 <[email protected]>
...
> > > > Don't you have a dependency issue here?
> > >
> > > Okay, I will explain the need for the changes done in commit description.
> >
> > What I meant is that the 8250_pci patch should be _prerequisite_ to your
> > stuff and not otherwise.
>
> Hi Andy,
> So, do you suggest having these changes done as first patch of the patchset prior to patches
> specific for our driver?
Yes.
--
With Best Regards,
Andy Shevchenko
> From: Andy Shevchenko <[email protected]>
> Sent: Thursday, November 17, 2022 1:55 PM
> To: Kumaravel Thiagarajan - I21417
> <[email protected]>
> Subject: Re: [PATCH v5 tty-next 1/4] 8250: microchip: pci1xxxx: Add driver for
> quad-uart support.
>
> > + case PCI_SUBDEVICE_ID_EFAR_PCI11414:
> > + irq_idx = idx;
> > + break;
>
> Try to make this entire switch-case more compact. It's possible.
Hi Andy,
I am planning to use look-up table for this in-order to avoid computation within switch case. Does this approach sound good?
Thanks,
Tharun Kumar P
On Fri, Nov 25, 2022 at 03:45:01PM +0000, [email protected] wrote:
> > From: Andy Shevchenko <[email protected]>
> > Sent: Thursday, November 17, 2022 1:55 PM
...
> > > + case PCI_SUBDEVICE_ID_EFAR_PCI11414:
> > > + irq_idx = idx;
> > > + break;
> >
> > Try to make this entire switch-case more compact. It's possible.
>
> I am planning to use look-up table for this in-order to avoid computation
> within switch case. Does this approach sound good?
Maybe, it's you who decides, just show us the result and we will see.
--
With Best Regards,
Andy Shevchenko