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 are mentioned in each patch in the patchset.
Thanks to Andy Shevchenko, Ilpo Jarvinen, Geert Uytterhoeven for their
review comments for v1.
Kumaravel Thiagarajan (3):
8250: microchip: pci1xxxx: Add driver for quad-uart support.
8250: microchip: pci1xxxx: Add rs485 support to quad-uart driver.
8250: microchip: pci1xxxx: Add power management functions to quad-uart
driver.
MAINTAINERS | 6 +
drivers/tty/serial/8250/8250_pci1xxxx.c | 563 ++++++++++++++++++++++++
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, 591 insertions(+)
create mode 100644 drivers/tty/serial/8250/8250_pci1xxxx.c
--
2.25.1
pci1xxxx uart supports rs485 mode of operation in the hardware with
auto-direction control with configurable delay for releasing RTS after
the transmission. This patch adds support for the rs485 mode.
Signed-off-by: Kumaravel Thiagarajan <[email protected]>
---
Changes in v2:
- move pci1xxxx_rs485_config to a separate patch with
pci1xxxx_rs485_supported.
---
drivers/tty/serial/8250/8250_pci1xxxx.c | 57 +++++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
index 41a4b94f52b4..999e5a284266 100644
--- a/drivers/tty/serial/8250/8250_pci1xxxx.c
+++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
@@ -137,6 +137,61 @@ static void pci1xxxx_set_divisor(struct uart_port *port, unsigned int baud,
writel((quot << 8) | frac, (port->membase + CLK_DIVISOR_REG));
}
+static const struct serial_rs485 pci1xxxx_rs485_supported = {
+ .flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RTS_AFTER_SEND,
+ .delay_rts_after_send = 1,
+ /* Delay RTS before send is not supported */
+};
+
+static int pci1xxxx_rs485_config(struct uart_port *port,
+ struct ktermios *termios,
+ struct serial_rs485 *rs485)
+{
+ u32 clock_div = readl(port->membase + CLK_DIVISOR_REG);
+ u8 delay_in_baud_periods = 0;
+ u32 baud_period_in_ns = 0;
+ u8 data = 0;
+
+ /* pci1xxxx's uart hardware supports only RTS delay after
+ * Tx and in units of bit times to a maximum of 15
+ */
+
+ rs485->flags &= SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
+ SER_RS485_RTS_AFTER_SEND;
+
+ if (rs485->flags & SER_RS485_ENABLED) {
+ memset(rs485->padding, 0, sizeof(rs485->padding));
+
+ data = ADCL_CFG_EN | ADCL_CFG_PIN_SEL;
+
+ if (!(rs485->flags & SER_RS485_RTS_ON_SEND)) {
+ data |= ADCL_CFG_POL_SEL;
+ rs485->flags |= SER_RS485_RTS_AFTER_SEND;
+ } else {
+ rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
+ }
+
+ if (rs485->delay_rts_after_send) {
+ baud_period_in_ns = ((clock_div >> 8) * 16);
+ delay_in_baud_periods = (rs485->delay_rts_after_send * 1000000) /
+ baud_period_in_ns;
+ if (delay_in_baud_periods > 0xF)
+ delay_in_baud_periods = 0xF;
+ data |= delay_in_baud_periods << 8;
+ rs485->delay_rts_after_send = (baud_period_in_ns * delay_in_baud_periods) /
+ 1000000;
+ rs485->delay_rts_before_send = 0;
+ }
+ } else {
+ memset(rs485, 0, sizeof(*rs485));
+ }
+
+ writeb(data, (port->membase + ADCL_CFG_REG));
+ port->rs485 = *rs485;
+
+ return 0;
+}
+
static int pci1xxxx_get_num_ports(struct pci_dev *dev)
{
int nr_ports = 1;
@@ -217,6 +272,8 @@ 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;
+ port->port.rs485_config = pci1xxxx_rs485_config;
+ port->port.rs485_supported = pci1xxxx_rs485_supported;
ret = pci1xxxx_setup_port(priv, port, offset);
if (ret < 0)
return ret;
--
2.25.1
pci1xxxx's quad-uart function has the capability to wake up the host from
suspend state. Enable wakeup before entering into suspend and disable
wakeup on resume.
Signed-off-by: Kumaravel Thiagarajan <[email protected]>
---
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 | 112 ++++++++++++++++++++++++
1 file changed, 112 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
index 999e5a284266..0a0459f66177 100644
--- a/drivers/tty/serial/8250/8250_pci1xxxx.c
+++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
@@ -352,6 +352,112 @@ static void pci1xxxx_irq_assign(struct pci1xxxx_8250 *priv,
}
}
+static bool pci1xxxx_port_suspend(int line)
+{
+ struct uart_8250_port *up = serial8250_get_port(line);
+ struct uart_port *port = &up->port;
+ unsigned long flags;
+ u8 wakeup_mask;
+ bool ret = false;
+
+ 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);
+
+ if ((wakeup_mask & UART_WAKE_SRCS) != UART_WAKE_SRCS)
+ ret = true;
+ }
+
+ writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
+
+ return ret;
+}
+
+static void pci1xxxx_port_resume(int line)
+{
+ struct uart_8250_port *up = serial8250_get_port(line);
+ struct uart_port *port = &up->port;
+ unsigned long flags;
+
+ 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);
+ }
+}
+
+static int pci1xxxx_suspend(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;
+ bool wakeup = false;
+ 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_serial_probe(struct pci_dev *dev,
const struct pci_device_id *ent)
{
@@ -427,6 +533,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_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11010) },
{ PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11101) },
@@ -441,6 +550,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,
};
--
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.
Signed-off-by: Kumaravel Thiagarajan <[email protected]>
---
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 | 6 +
drivers/tty/serial/8250/8250_pci1xxxx.c | 394 ++++++++++++++++++++++++
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, 422 insertions(+)
create mode 100644 drivers/tty/serial/8250/8250_pci1xxxx.c
diff --git a/MAINTAINERS b/MAINTAINERS
index d30f26e07cd3..3390693d57ae 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -218,6 +218,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
F: drivers/tty/serial/8250*
F: include/linux/serial_8250.h
+MICROCHIP PCIe UART DRIVER
+M: Kumaravel Thiagarajan <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/tty/serial/8250/8250_pci1xxxx.c
+
8390 NETWORK DRIVERS [WD80x3/SMC-ELITE, SMC-ULTRA, NE2000, 3C503, etc.]
L: [email protected]
S: Orphan / Obsolete
diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
new file mode 100644
index 000000000000..41a4b94f52b4
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
@@ -0,0 +1,394 @@
+// 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/serial_core.h>
+#include <linux/8250_pci.h>
+#include <asm/byteorder.h>
+#include <linux/bitops.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/pci.h>
+#include <linux/tty.h>
+#include <linux/io.h>
+#include "8250.h"
+
+#define PCI_VENDOR_ID_MCHP_PCI1XXXX 0x1055
+
+#define PCI_DEVICE_ID_MCHP_PCI12000 0xA002
+#define PCI_DEVICE_ID_MCHP_PCI11010 0xA012
+#define PCI_DEVICE_ID_MCHP_PCI11101 0xA022
+#define PCI_DEVICE_ID_MCHP_PCI11400 0xA032
+#define PCI_DEVICE_ID_MCHP_PCI11414 0xA042
+
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p 0x0001
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p012 0x0002
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013 0x0003
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023 0x0004
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123 0x0005
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p01 0x0006
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02 0x0007
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03 0x0008
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12 0x0009
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13 0x000A
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23 0x000B
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p0 0x000C
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1 0x000D
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2 0x000E
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3 0x000F
+
+#define PCI_SUBDEVICE_ID_MCHP_PCI12000 0xA002
+#define PCI_SUBDEVICE_ID_MCHP_PCI11010 0xA012
+#define PCI_SUBDEVICE_ID_MCHP_PCI11101 0xA022
+#define PCI_SUBDEVICE_ID_MCHP_PCI11400 0xA032
+#define PCI_SUBDEVICE_ID_MCHP_PCI11414 0xA042
+
+#define UART_ACTV_REG 0x11
+#define UART_ACTV_SET_ACTIVE 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 CLK_SEL_REG 0x50
+#define CLK_SEL_MASK GENMASK(1, 0)
+#define CLK_SEL_166MHZ 0x01
+#define CLK_SEL_500MHZ 0x02
+
+#define CLK_DIVISOR_REG 0x54
+
+#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 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_RESET_REG 0x94
+#define UART_RESET_D3_RESET_DISABLE BIT(16)
+
+#define UART_BIT_SAMPLE_CNT 16
+
+struct pci1xxxx_8250 {
+ struct pci_dev *dev;
+ unsigned int nr;
+ void __iomem *membase;
+ int line[];
+};
+
+static int pci1xxxx_setup_port(struct pci1xxxx_8250 *priv, struct uart_8250_port *port,
+ int offset)
+{
+ struct pci_dev *dev = priv->dev;
+
+ if (pci_resource_flags(dev, 0) & IORESOURCE_MEM) {
+ if (!pcim_iomap(dev, 0, 0) && !pcim_iomap_table(dev))
+ return -ENOMEM;
+
+ port->port.iotype = UPIO_MEM;
+ port->port.iobase = 0;
+ port->port.mapbase = pci_resource_start(dev, 0) + offset;
+ port->port.membase = pcim_iomap_table(dev)[0] + offset;
+ port->port.regshift = 0;
+ } else {
+ port->port.iotype = UPIO_PORT;
+ port->port.iobase = pci_resource_start(dev, 0) + offset;
+ port->port.mapbase = 0;
+ port->port.membase = NULL;
+ port->port.regshift = 0;
+ }
+
+ return 0;
+}
+
+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 nano seconds.
+ * Fractional part x denotes x/255 parts of a nano second.
+ */
+
+ quot = ((1000000000) / (baud * UART_BIT_SAMPLE_CNT));
+ *frac = (((1000000000 - (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((quot << 8) | frac, (port->membase + CLK_DIVISOR_REG));
+}
+
+static int pci1xxxx_get_num_ports(struct pci_dev *dev)
+{
+ int nr_ports = 1;
+
+ switch (dev->subsystem_device) {
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p0:
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1:
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2:
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3:
+ nr_ports = 1;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p01:
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02:
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03:
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12:
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13:
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23:
+ nr_ports = 2;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p012:
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123:
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013:
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023:
+ nr_ports = 3;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p:
+ case PCI_SUBDEVICE_ID_MCHP_PCI11414:
+ nr_ports = 4;
+ break;
+ }
+
+ return nr_ports;
+}
+
+static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
+ struct uart_8250_port *port, int idx)
+{
+ int first_offset = 0;
+ int offset;
+ int ret;
+
+ switch (priv->dev->subsystem_device) {
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1:
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12:
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123:
+ first_offset = 256;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2:
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23:
+ first_offset = 512;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3:
+ first_offset = 768;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02:
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023:
+ if (idx > 0)
+ idx++;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03:
+ if (idx > 0)
+ idx += 2;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13:
+ first_offset = 256;
+ if (idx > 0)
+ idx++;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_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;
+ ret = pci1xxxx_setup_port(priv, port, offset);
+ if (ret < 0)
+ return ret;
+
+ writeb(UART_ACTV_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)
+{
+ switch (priv->dev->subsystem_device) {
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p0:
+ case PCI_SUBDEVICE_ID_MCHP_PCI12000:
+ case PCI_SUBDEVICE_ID_MCHP_PCI11010:
+ case PCI_SUBDEVICE_ID_MCHP_PCI11101:
+ case PCI_SUBDEVICE_ID_MCHP_PCI11400:
+ uart->port.irq = pci_irq_vector(priv->dev, 0);
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1:
+ uart->port.irq = pci_irq_vector(priv->dev, 1);
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2:
+ uart->port.irq = pci_irq_vector(priv->dev, 2);
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3:
+ uart->port.irq = pci_irq_vector(priv->dev, 3);
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p01:
+ uart->port.irq = pci_irq_vector(priv->dev, idx);
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02:
+ if (idx > 0)
+ idx++;
+ uart->port.irq = pci_irq_vector(priv->dev, idx);
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03:
+ if (idx > 0)
+ idx += 2;
+ uart->port.irq = pci_irq_vector(priv->dev, idx);
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12:
+ uart->port.irq = pci_irq_vector(priv->dev, idx + 1);
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13:
+ if (idx > 0)
+ idx += 1;
+ uart->port.irq = pci_irq_vector(priv->dev, idx + 1);
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23:
+ uart->port.irq = pci_irq_vector(priv->dev, idx + 2);
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p012:
+ uart->port.irq = pci_irq_vector(priv->dev, idx);
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013:
+ if (idx > 1)
+ idx++;
+ uart->port.irq = pci_irq_vector(priv->dev, idx);
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023:
+ if (idx > 0)
+ idx++;
+ uart->port.irq = pci_irq_vector(priv->dev, idx);
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123:
+ uart->port.irq = pci_irq_vector(priv->dev, idx + 1);
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p:
+ case PCI_SUBDEVICE_ID_MCHP_PCI11414:
+ uart->port.irq = pci_irq_vector(priv->dev, idx);
+ break;
+ }
+}
+
+static int pci1xxxx_serial_probe(struct pci_dev *dev,
+ const struct pci_device_id *ent)
+{
+ struct pci1xxxx_8250 *priv;
+ struct uart_8250_port uart;
+ unsigned int nr_ports, i;
+ int num_vectors = 0;
+ int rc;
+
+ rc = pcim_enable_device(dev);
+ if (rc)
+ return rc;
+
+ nr_ports = pci1xxxx_get_num_ports(dev);
+
+ priv = devm_kzalloc(&dev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->membase = pcim_iomap(dev, 0, 0);
+ priv->dev = dev;
+ priv->nr = nr_ports;
+
+ pci_set_master(dev);
+
+ num_vectors = pci_alloc_irq_vectors(dev, 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_TYPE | UPF_FIXED_PORT;
+ uart.port.uartclk = 62500000;
+ uart.port.dev = &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(dev, 0);
+
+ for (i = 0; i < nr_ports; i++) {
+ if (num_vectors == 4)
+ pci1xxxx_irq_assign(priv, &uart, i);
+ priv->line[i] = -ENOSPC;
+ rc = pci1xxxx_setup(priv, &uart, i);
+ if (rc) {
+ dev_err(&dev->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->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(dev, 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_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11010) },
+ { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11101) },
+ { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11400) },
+ { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11414) },
+ { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_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_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..a886727ff7bf 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -528,6 +528,16 @@ config SERIAL_8250_TEGRA
Select this option if you have machine with an NVIDIA Tegra SoC and
wish to enable 8250 serial driver for the Tegra serial interfaces.
+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.
+
config SERIAL_8250_BCM7271
tristate "Broadcom 8250 based serial port"
depends on SERIAL_8250 && (ARCH_BRCMSTB || COMPILE_TEST)
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index bee908f99ea0..e900ff11e321 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_SERIAL_8250_LPSS) += 8250_lpss.o
obj-$(CONFIG_SERIAL_8250_MID) += 8250_mid.o
obj-$(CONFIG_SERIAL_8250_PERICOM) += 8250_pericom.o
obj-$(CONFIG_SERIAL_8250_PXA) += 8250_pxa.o
+obj-$(CONFIG_SERIAL_8250_PCI1XXXX) += 8250_pci1xxxx.o
obj-$(CONFIG_SERIAL_8250_TEGRA) += 8250_tegra.o
obj-$(CONFIG_SERIAL_8250_BCM7271) += 8250_bcm7271.o
obj-$(CONFIG_SERIAL_OF_PLATFORM) += 8250_of.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
On Sat, 1 Oct 2022, Kumaravel Thiagarajan wrote:
> pci1xxxx uart supports rs485 mode of operation in the hardware with
> auto-direction control with configurable delay for releasing RTS after
> the transmission. This patch adds support for the rs485 mode.
>
> Signed-off-by: Kumaravel Thiagarajan <[email protected]>
> ---
> Changes in v2:
> - move pci1xxxx_rs485_config to a separate patch with
> pci1xxxx_rs485_supported.
> ---
> drivers/tty/serial/8250/8250_pci1xxxx.c | 57 +++++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
> index 41a4b94f52b4..999e5a284266 100644
> --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> @@ -137,6 +137,61 @@ static void pci1xxxx_set_divisor(struct uart_port *port, unsigned int baud,
> writel((quot << 8) | frac, (port->membase + CLK_DIVISOR_REG));
> }
>
> +static const struct serial_rs485 pci1xxxx_rs485_supported = {
> + .flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RTS_AFTER_SEND,
> + .delay_rts_after_send = 1,
> + /* Delay RTS before send is not supported */
> +};
> +
> +static int pci1xxxx_rs485_config(struct uart_port *port,
> + struct ktermios *termios,
> + struct serial_rs485 *rs485)
> +{
> + u32 clock_div = readl(port->membase + CLK_DIVISOR_REG);
> + u8 delay_in_baud_periods = 0;
> + u32 baud_period_in_ns = 0;
> + u8 data = 0;
> +
> + /* pci1xxxx's uart hardware supports only RTS delay after
> + * Tx and in units of bit times to a maximum of 15
> + */
> +
> + rs485->flags &= SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> + SER_RS485_RTS_AFTER_SEND;
Drop this, core handles it for you.
I think I already mentioned to you that there is a lot of stuff that core
handles for you.
> + if (rs485->flags & SER_RS485_ENABLED) {
> + memset(rs485->padding, 0, sizeof(rs485->padding));
Core handles this for you.
> + data = ADCL_CFG_EN | ADCL_CFG_PIN_SEL;
> +
> + if (!(rs485->flags & SER_RS485_RTS_ON_SEND)) {
> + data |= ADCL_CFG_POL_SEL;
> + rs485->flags |= SER_RS485_RTS_AFTER_SEND;
> + } else {
> + rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
> + }
Core handles that flags sanitization for you.
> + if (rs485->delay_rts_after_send) {
> + baud_period_in_ns = ((clock_div >> 8) * 16);
If that >> 8 is there to take part of the CLK_DIVISOR_REG register's bits,
you want to define a mask and use FIELD_GET().
> + delay_in_baud_periods = (rs485->delay_rts_after_send * 1000000) /
NSEC_PER_MSEC?
> + baud_period_in_ns;
> + if (delay_in_baud_periods > 0xF)
> + delay_in_baud_periods = 0xF;
> + data |= delay_in_baud_periods << 8;
You want to add something along these lines:
#define ADCL_CFG_RTS_DELAY_MASK GENMASK(11, 8)
Then do:
delay_in_baud_periods = min(delay_in_baud_periods,
FIELD_MAX(ADCL_CFG_RTS_DELAY_MASK));
data |= FIELD_PREP(ADCL_CFG_RTS_DELAY_MASK, delay_in_baud_periods);
> + rs485->delay_rts_after_send = (baud_period_in_ns * delay_in_baud_periods) /
> + 1000000;
NSEC_PER_MSEC?
> + rs485->delay_rts_before_send = 0;
> + }
> + } else {
> + memset(rs485, 0, sizeof(*rs485));
Core handles this.
> + }
> +
> + writeb(data, (port->membase + ADCL_CFG_REG));
> + port->rs485 = *rs485;
Core handles this.
> + return 0;
> +}
> +
> static int pci1xxxx_get_num_ports(struct pci_dev *dev)
> {
> int nr_ports = 1;
> @@ -217,6 +272,8 @@ 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;
> + port->port.rs485_config = pci1xxxx_rs485_config;
> + port->port.rs485_supported = pci1xxxx_rs485_supported;
> ret = pci1xxxx_setup_port(priv, port, offset);
> if (ret < 0)
> return ret;
>
--
i.
On Sat, Oct 1, 2022 at 9:15 AM Kumaravel Thiagarajan
<[email protected]> wrote:
>
> pci1xxxx's quad-uart function has the capability to wake up the host from
> suspend state. Enable wakeup before entering into suspend and disable
> wakeup on resume.
...
> + port->suspended == 0
How is this check race-protected?
...
> +static void pci1xxxx_port_resume(int line)
> +{
> + if (port->suspended == 0) {
Ditto.
> + }
> +}
...
If you have similarities with 8250_pci, probably you need to split it
to 8250_pcilib.c and share. (See how 8250_dw /8250_lpss are done in
that sense.)
--
With Best Regards,
Andy Shevchenko
On Sat, 1 Oct 2022, Kumaravel Thiagarajan wrote:
> pci1xxxx uart supports rs485 mode of operation in the hardware with
> auto-direction control with configurable delay for releasing RTS after
> the transmission. This patch adds support for the rs485 mode.
>
> Signed-off-by: Kumaravel Thiagarajan <[email protected]>
> ---
> Changes in v2:
> - move pci1xxxx_rs485_config to a separate patch with
> pci1xxxx_rs485_supported.
> ---
> drivers/tty/serial/8250/8250_pci1xxxx.c | 57 +++++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
> index 41a4b94f52b4..999e5a284266 100644
> --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> +
> + if (rs485->delay_rts_after_send) {
> + baud_period_in_ns = ((clock_div >> 8) * 16);
Is this 16 perhaps UART_BIT_SAMPLE_CNT?
--
i.
On Sat, Oct 1, 2022 at 9:15 AM Kumaravel Thiagarajan
<[email protected]> 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.
Thanks for an update, my comments below.
...
> +MICROCHIP PCIe UART DRIVER
> +M: Kumaravel Thiagarajan <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/tty/serial/8250/8250_pci1xxxx.c
> 8390 NETWORK DRIVERS [WD80x3/SMC-ELITE, SMC-ULTRA, NE2000, 3C503, etc.]
This doesn't look ordered at all. Please, locate your section in the
appropriate place.
...
> +#include <linux/serial_core.h>
> +#include <linux/8250_pci.h>
> +#include <asm/byteorder.h>
> +#include <linux/bitops.h>
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/pci.h>
> +#include <linux/tty.h>
> +#include <linux/io.h>
Please, keep this ordered and put a blank line between linux/* and
asm/* and "(local)" inclusions.
> +#include "8250.h"
...
> +#define PCI_VENDOR_ID_MCHP_PCI1XXXX 0x1055
Vendor should be vendor, this macro doesn't look right. See pci_ids.h
on how to properly define it.
Btw, PCI_VENDOR_ID_EFAR 0x1055 is there. Use it. (I believe microchip
bought that company in the past?)
...
> +#define PCI_DEVICE_ID_MCHP_PCI12000 0xA002
> +#define PCI_DEVICE_ID_MCHP_PCI11010 0xA012
> +#define PCI_DEVICE_ID_MCHP_PCI11101 0xA022
> +#define PCI_DEVICE_ID_MCHP_PCI11400 0xA032
> +#define PCI_DEVICE_ID_MCHP_PCI11414 0xA042
Use PCI_DEVICE_ID_#vendor_#device format. In a similar way for subdevice IDs.
...
> +#define UART_ACTV_REG 0x11
> +#define UART_ACTV_SET_ACTIVE BIT(0)
This namespace...
> +#define CLK_SEL_REG 0x50
> +#define CLK_SEL_MASK GENMASK(1, 0)
> +#define CLK_SEL_166MHZ 0x01
> +#define CLK_SEL_500MHZ 0x02
> +#define CLK_DIVISOR_REG 0x54
...and this one are potentially conflicting with more generic ones.
Ditto for the rest of the similar definitions.
...
> +static int pci1xxxx_setup_port(struct pci1xxxx_8250 *priv, struct uart_8250_port *port,
> + int offset)
> +{
> + struct pci_dev *dev = priv->dev;
> +
> + if (pci_resource_flags(dev, 0) & IORESOURCE_MEM) {
> + if (!pcim_iomap(dev, 0, 0) && !pcim_iomap_table(dev))
> + return -ENOMEM;
> +
> + port->port.iotype = UPIO_MEM;
> + port->port.iobase = 0;
> + port->port.mapbase = pci_resource_start(dev, 0) + offset;
> + port->port.membase = pcim_iomap_table(dev)[0] + offset;
> + port->port.regshift = 0;
> + } else {
> + port->port.iotype = UPIO_PORT;
> + port->port.iobase = pci_resource_start(dev, 0) + offset;
> + port->port.mapbase = 0;
> + port->port.membase = NULL;
> + port->port.regshift = 0;
> + }
> +
> + return 0;
> +}
Do you really have cards that are providing IO ports? If not, simplify
this accordingly.
...
> + /*
> + * Calculate baud rate sampling period in nano seconds.
nanoseconds
> + * Fractional part x denotes x/255 parts of a nano second.
> + */
...
> + quot = ((1000000000) / (baud * UART_BIT_SAMPLE_CNT));
> + *frac = (((1000000000 - (quot * baud * UART_BIT_SAMPLE_CNT)) /
> + UART_BIT_SAMPLE_CNT) * 255) / baud;
NSEC_PER_SEC ?
...
> + writel((quot << 8) | frac, (port->membase + CLK_DIVISOR_REG));
Too many parentheses.
...
> +static int pci1xxxx_get_num_ports(struct pci_dev *dev)
> +{
> + int nr_ports = 1;
Make this default case.
> +
> + switch (dev->subsystem_device) {
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p0:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3:
> + nr_ports = 1;
> + break;
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p01:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23:
> + nr_ports = 2;
> + break;
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p012:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023:
> + nr_ports = 3;
> + break;
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p:
> + case PCI_SUBDEVICE_ID_MCHP_PCI11414:
> + nr_ports = 4;
> + break;
> + }
> + return nr_ports;
Drop the unnecessary local variable and use return directly from the
switch-cases.
> +}
...
> + int first_offset = 0;
Use default switch-case.
...
> + offset = first_offset + (idx * 256);
Too many parentheses. Check all your code for this kind of unnecessary.
...
> + switch (priv->dev->subsystem_device) {
> + case PCI_SUBDEVICE_ID_MCHP_PCI11414:
> + uart->port.irq = pci_irq_vector(priv->dev, idx);
> + break;
default?
> + }
...
> + uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_TYPE | UPF_FIXED_PORT;
Isn't there something duplicating here what's done in ->setup()?
...
> + uart.port.uartclk = 62500000;
HZ_PER_MHZ ?
...
> + if (num_vectors == 4)
This check should take care of all possible MSI >= 2, correct?
> + writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI, (priv->membase + UART_PCI_CTRL_REG));
> + else
> + uart.port.irq = pci_irq_vector(dev, 0);
...
> + for (i = 0; i < nr_ports; i++) {
> + if (num_vectors == 4)
Ditto.
> + pci1xxxx_irq_assign(priv, &uart, i);
> + priv->line[i] = -ENOSPC;
> + rc = pci1xxxx_setup(priv, &uart, i);
> + if (rc) {
> + dev_err(&dev->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->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;
> + }
> + }
...
> + [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,
> + },
Can you assign this in ->setup() or so instead of adding a new port type?
> };
...
> +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.
Keep it in the Quad UART group of config sections (Yes, I know that
not all of them are sorted there, but try your best).
...
> @@ -40,6 +40,7 @@ obj-$(CONFIG_SERIAL_8250_LPSS) += 8250_lpss.o
> obj-$(CONFIG_SERIAL_8250_MID) += 8250_mid.o
> obj-$(CONFIG_SERIAL_8250_PERICOM) += 8250_pericom.o
> obj-$(CONFIG_SERIAL_8250_PXA) += 8250_pxa.o
> +obj-$(CONFIG_SERIAL_8250_PCI1XXXX) += 8250_pci1xxxx.o
> obj-$(CONFIG_SERIAL_8250_TEGRA) += 8250_tegra.o
> obj-$(CONFIG_SERIAL_8250_BCM7271) += 8250_bcm7271.o
> obj-$(CONFIG_SERIAL_OF_PLATFORM) += 8250_of.o
Ditto.
--
With Best Regards,
Andy Shevchenko
On Sat, 1 Oct 2022, Kumaravel Thiagarajan wrote:
> pci1xxxx's quad-uart function has the capability to wake up the host from
> suspend state. Enable wakeup before entering into suspend and disable
> wakeup on resume.
>
> Signed-off-by: Kumaravel Thiagarajan <[email protected]>
> ---
> 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 | 112 ++++++++++++++++++++++++
> 1 file changed, 112 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
> index 999e5a284266..0a0459f66177 100644
> --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> @@ -352,6 +352,112 @@ static void pci1xxxx_irq_assign(struct pci1xxxx_8250 *priv,
> }
> }
>
> +static bool pci1xxxx_port_suspend(int line)
> +{
> + struct uart_8250_port *up = serial8250_get_port(line);
> + struct uart_port *port = &up->port;
> + unsigned long flags;
> + u8 wakeup_mask;
> + bool ret = false;
> +
> + 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);
> +
> + if ((wakeup_mask & UART_WAKE_SRCS) != UART_WAKE_SRCS)
> + ret = true;
> + }
> +
> + writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> +
> + return ret;
> +}
> +
> +static void pci1xxxx_port_resume(int line)
> +{
> + struct uart_8250_port *up = serial8250_get_port(line);
> + struct uart_port *port = &up->port;
> + unsigned long flags;
> +
> + writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> +
> + if (port->suspended == 0) {
Is this check the right way around?
> + spin_lock_irqsave(&port->lock, flags);
> + port->mctrl |= TIOCM_OUT2;
> + port->ops->set_mctrl(port, port->mctrl);
> + spin_unlock_irqrestore(&port->lock, flags);
> + }
> +}
> +
> +static int pci1xxxx_suspend(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;
> + bool wakeup = false;
> + 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]);
So first serial8250_suspend_port() calls into uart_suspend_port() that
sets port->suspended to 1, then pci1xxxx_port_suspend() checks if it's 0.
Is this intentional?
--
i.
On Sat, 1 Oct 2022, 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.
>
> Signed-off-by: Kumaravel Thiagarajan <[email protected]>
> ---
> 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 | 6 +
> drivers/tty/serial/8250/8250_pci1xxxx.c | 394 ++++++++++++++++++++++++
> 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, 422 insertions(+)
> create mode 100644 drivers/tty/serial/8250/8250_pci1xxxx.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d30f26e07cd3..3390693d57ae 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -218,6 +218,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
> F: drivers/tty/serial/8250*
> F: include/linux/serial_8250.h
>
> +MICROCHIP PCIe UART DRIVER
> +M: Kumaravel Thiagarajan <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/tty/serial/8250/8250_pci1xxxx.c
> +
> 8390 NETWORK DRIVERS [WD80x3/SMC-ELITE, SMC-ULTRA, NE2000, 3C503, etc.]
> L: [email protected]
> S: Orphan / Obsolete
> diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
> new file mode 100644
> index 000000000000..41a4b94f52b4
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> @@ -0,0 +1,394 @@
> +#define PCI_VENDOR_ID_MCHP_PCI1XXXX 0x1055
> +
> +#define PCI_DEVICE_ID_MCHP_PCI12000 0xA002
> +#define PCI_DEVICE_ID_MCHP_PCI11010 0xA012
> +#define PCI_DEVICE_ID_MCHP_PCI11101 0xA022
> +#define PCI_DEVICE_ID_MCHP_PCI11400 0xA032
> +#define PCI_DEVICE_ID_MCHP_PCI11414 0xA042
> +
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p 0x0001
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p012 0x0002
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013 0x0003
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023 0x0004
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123 0x0005
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p01 0x0006
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02 0x0007
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03 0x0008
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12 0x0009
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13 0x000A
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23 0x000B
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p0 0x000C
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1 0x000D
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2 0x000E
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3 0x000F
> +
> +#define PCI_SUBDEVICE_ID_MCHP_PCI12000 0xA002
> +#define PCI_SUBDEVICE_ID_MCHP_PCI11010 0xA012
> +#define PCI_SUBDEVICE_ID_MCHP_PCI11101 0xA022
> +#define PCI_SUBDEVICE_ID_MCHP_PCI11400 0xA032
> +#define PCI_SUBDEVICE_ID_MCHP_PCI11414 0xA042
Usually lowercase is used for hexadecimal letters.
> +#define UART_ACTV_REG 0x11
> +#define UART_ACTV_SET_ACTIVE 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 CLK_SEL_REG 0x50
> +#define CLK_SEL_MASK GENMASK(1, 0)
> +#define CLK_SEL_166MHZ 0x01
> +#define CLK_SEL_500MHZ 0x02
FIELD_PREP(CLK_SEL_MASK, ..) for thse two.
> +
> +#define CLK_DIVISOR_REG 0x54
> +
> +#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 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_RESET_REG 0x94
> +#define UART_RESET_D3_RESET_DISABLE BIT(16)
> +
> +#define UART_BIT_SAMPLE_CNT 16
> +
> +struct pci1xxxx_8250 {
> + struct pci_dev *dev;
> + unsigned int nr;
> + void __iomem *membase;
> + int line[];
> +};
> +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 nano seconds.
> + * Fractional part x denotes x/255 parts of a nano second.
> + */
> +
> + quot = ((1000000000) / (baud * UART_BIT_SAMPLE_CNT));
> + *frac = (((1000000000 - (quot * baud * UART_BIT_SAMPLE_CNT)) /
NSEC_PER_SEC
> + 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((quot << 8) | frac, (port->membase + CLK_DIVISOR_REG));
Define mask for quotient part and use FIELD_PREP().
Remove extra parenthesis.
> +static int pci1xxxx_serial_probe(struct pci_dev *dev,
> + const struct pci_device_id *ent)
> +{
> + struct pci1xxxx_8250 *priv;
> + struct uart_8250_port uart;
> + unsigned int nr_ports, i;
> + int num_vectors = 0;
> + int rc;
> +
> + rc = pcim_enable_device(dev);
> + if (rc)
> + return rc;
> +
> + nr_ports = pci1xxxx_get_num_ports(dev);
> +
> + priv = devm_kzalloc(&dev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->membase = pcim_iomap(dev, 0, 0);
> + priv->dev = dev;
> + priv->nr = nr_ports;
Extra space.
> +
> + pci_set_master(dev);
> +
> + num_vectors = pci_alloc_irq_vectors(dev, 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_TYPE | UPF_FIXED_PORT;
> + uart.port.uartclk = 62500000;
> + uart.port.dev = &dev->dev;
> +
> + if (num_vectors == 4)
> + writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI, (priv->membase + UART_PCI_CTRL_REG));
Extra parenthesis.
--
i.
Le 01/10/2022 à 08:15, Kumaravel Thiagarajan a écrit :
> 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.
>
> Signed-off-by: Kumaravel Thiagarajan <[email protected]>
> ---
[...]
> +static int pci1xxxx_setup_port(struct pci1xxxx_8250 *priv, struct uart_8250_port *port,
> + int offset)
> +{
> + struct pci_dev *dev = priv->dev;
> +
> + if (pci_resource_flags(dev, 0) & IORESOURCE_MEM) {
> + if (!pcim_iomap(dev, 0, 0) && !pcim_iomap_table(dev))
> + return -ENOMEM;
> +
> + port->port.iotype = UPIO_MEM;
> + port->port.iobase = 0;
> + port->port.mapbase = pci_resource_start(dev, 0) + offset;
> + port->port.membase = pcim_iomap_table(dev)[0] + offset;
Hi,
Is it needed to call pcim_iomap_table(dev) twice? (here and a few lines
above in the 'if')
> + port->port.regshift = 0;
> + } else {
> + port->port.iotype = UPIO_PORT;
> + port->port.iobase = pci_resource_start(dev, 0) + offset;
> + port->port.mapbase = 0;
> + port->port.membase = NULL;
> + port->port.regshift = 0;
> + }
> +
> + return 0;
> +}
[...]
> +static int pci1xxxx_serial_probe(struct pci_dev *dev,
> + const struct pci_device_id *ent)
> +{
> + struct pci1xxxx_8250 *priv;
> + struct uart_8250_port uart;
> + unsigned int nr_ports, i;
> + int num_vectors = 0;
> + int rc;
> +
> + rc = pcim_enable_device(dev);
> + if (rc)
> + return rc;
> +
> + nr_ports = pci1xxxx_get_num_ports(dev);
> +
> + priv = devm_kzalloc(&dev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->membase = pcim_iomap(dev, 0, 0);
> + priv->dev = dev;
> + priv->nr = nr_ports;
> +
> + pci_set_master(dev);
> +
> + num_vectors = pci_alloc_irq_vectors(dev, 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_TYPE | UPF_FIXED_PORT;
> + uart.port.uartclk = 62500000;
> + uart.port.dev = &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(dev, 0);
> +
> + for (i = 0; i < nr_ports; i++) {
> + if (num_vectors == 4)
> + pci1xxxx_irq_assign(priv, &uart, i);
> + priv->line[i] = -ENOSPC;
> + rc = pci1xxxx_setup(priv, &uart, i);
> + if (rc) {
> + dev_err(&dev->dev, "Failed to setup port %u\n", i);
> + break;
> + }
> + priv->line[i] = serial8250_register_8250_port(&uart);
In case of error, this should be undone in an error handling path in the
probe, as done in the remove() function below.
If we break, we still continue and return success. But the last
priv->line[i] are still 0. Is it an issue when pci1xxxx_serial_remove()
is called?
> +
> + if (priv->line[i] < 0) {
> + dev_err(&dev->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(dev, 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]);
> + }
> +}
> +
[...]
On Mon, Oct 3, 2022 at 10:36 PM Christophe JAILLET
<[email protected]> wrote:
> Le 01/10/2022 à 08:15, Kumaravel Thiagarajan a écrit :
...
> > + if (pci_resource_flags(dev, 0) & IORESOURCE_MEM) {
> > + if (!pcim_iomap(dev, 0, 0) && !pcim_iomap_table(dev))
> > + return -ENOMEM;
> > +
> > + port->port.iotype = UPIO_MEM;
> > + port->port.iobase = 0;
> > + port->port.mapbase = pci_resource_start(dev, 0) + offset;
> > + port->port.membase = pcim_iomap_table(dev)[0] + offset;
> Is it needed to call pcim_iomap_table(dev) twice? (here and a few lines
> above in the 'if')
Yes. But the main question I asked (if we really need IO ports
support) still remains.
> > + port->port.regshift = 0;
> > + } else {
> > + port->port.iotype = UPIO_PORT;
> > + port->port.iobase = pci_resource_start(dev, 0) + offset;
> > + port->port.mapbase = 0;
> > + port->port.membase = NULL;
> > + port->port.regshift = 0;
> > + }
--
With Best Regards,
Andy Shevchenko
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Monday, October 3, 2022 2:57 PM
> To: Kumaravel Thiagarajan - I21417 <[email protected]>
> Subject: Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power
> management functions to quad-uart driver.
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Sat, Oct 1, 2022 at 9:15 AM Kumaravel Thiagarajan
> <[email protected]> wrote:
> >
> > pci1xxxx's quad-uart function has the capability to wake up the host
> > from suspend state. Enable wakeup before entering into suspend and
> > disable wakeup on resume.
>
> ...
>
> > + port->suspended == 0
>
> How is this check race-protected?
>
> ...
>
> > +static void pci1xxxx_port_resume(int line) {
>
> > + if (port->suspended == 0) {
>
> Ditto.
>
> > + }
> > +}
>
> ...
>
> If you have similarities with 8250_pci, probably you need to split it to
> 8250_pcilib.c and share. (See how 8250_dw /8250_lpss are done in that
> sense.)
I will review my code against all the above comments and get back to you.
Thank You.
Regards,
Kumar
> -----Original Message-----
> From: Ilpo J?rvinen <[email protected]>
> Sent: Monday, October 3, 2022 2:51 PM
> To: Kumaravel Thiagarajan - I21417 <[email protected]>
> Subject: Re: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485
> support to quad-uart driver.
>
> On Sat, 1 Oct 2022, Kumaravel Thiagarajan wrote:
>
> > pci1xxxx uart supports rs485 mode of operation in the hardware with
> > auto-direction control with configurable delay for releasing RTS after
> > the transmission. This patch adds support for the rs485 mode.
> >
> > Signed-off-by: Kumaravel Thiagarajan
> > <[email protected]>
> > ---
> > Changes in v2:
> > - move pci1xxxx_rs485_config to a separate patch with
> > pci1xxxx_rs485_supported.
> > ---
> > drivers/tty/serial/8250/8250_pci1xxxx.c | 57
> > +++++++++++++++++++++++++
> > 1 file changed, 57 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > index 41a4b94f52b4..999e5a284266 100644
> > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > +
> > + if (rs485->delay_rts_after_send) {
> > + baud_period_in_ns = ((clock_div >> 8) * 16);
>
> Is this 16 perhaps UART_BIT_SAMPLE_CNT?
Yes. Is there any macro definition for that? I could not find any definition in the above name.
Thank You.
Regards,
Kumar
> -----Original Message-----
> From: Ilpo J?rvinen <[email protected]>
> Sent: Monday, October 3, 2022 3:21 PM
> To: Kumaravel Thiagarajan - I21417 <[email protected]>
> Subject: Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power
> management functions to quad-uart driver.
>
> On Sat, 1 Oct 2022, Kumaravel Thiagarajan wrote:
>
> > pci1xxxx's quad-uart function has the capability to wake up the host
> > from suspend state. Enable wakeup before entering into suspend and
> > disable wakeup on resume.
> >
> > Signed-off-by: Kumaravel Thiagarajan
> > <[email protected]>
> > ---
> > 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 | 112
> > ++++++++++++++++++++++++
> > 1 file changed, 112 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > index 999e5a284266..0a0459f66177 100644
> > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > @@ -352,6 +352,112 @@ static void pci1xxxx_irq_assign(struct
> pci1xxxx_8250 *priv,
> > }
> > }
> >
> > +static bool pci1xxxx_port_suspend(int line) {
> > + struct uart_8250_port *up = serial8250_get_port(line);
> > + struct uart_port *port = &up->port;
> > + unsigned long flags;
> > + u8 wakeup_mask;
> > + bool ret = false;
> > +
> > + 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);
> > +
> > + if ((wakeup_mask & UART_WAKE_SRCS) != UART_WAKE_SRCS)
> > + ret = true;
> > + }
> > +
> > + writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> > +
> > + return ret;
> > +}
> > +
> > +static void pci1xxxx_port_resume(int line) {
> > + struct uart_8250_port *up = serial8250_get_port(line);
> > + struct uart_port *port = &up->port;
> > + unsigned long flags;
> > +
> > + writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> > +
> > + if (port->suspended == 0) {
>
> Is this check the right way around?
Yes. I think port->suspended is not set for wake-up capable ports and the code in this if block gets executed for those ports.
I will check this again.
>
> > + spin_lock_irqsave(&port->lock, flags);
> > + port->mctrl |= TIOCM_OUT2;
> > + port->ops->set_mctrl(port, port->mctrl);
> > + spin_unlock_irqrestore(&port->lock, flags);
> > + }
> > +}
> > +
> > +static int pci1xxxx_suspend(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;
> > + bool wakeup = false;
> > + 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]);
>
> So first serial8250_suspend_port() calls into uart_suspend_port() that sets
> port->suspended to 1, then pci1xxxx_port_suspend() checks if it's 0.
> Is this intentional?
Yes. I think port->suspended does not seem to be set for wake-up capable ports and only
for those ports, inside pci1xxxx_port_suspend, TIOCM_OUT2 is cleared.
But I must check for the race condition as Andy had pointed out.
Please let me know if there are any questions.
Thank You.
Regards,
Kumar
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Monday, October 3, 2022 2:53 PM
> To: Kumaravel Thiagarajan - I21417 <[email protected]>
> Subject: Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for
> quad-uart support.
>
> On Sat, Oct 1, 2022 at 9:15 AM Kumaravel Thiagarajan
> <[email protected]> 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.
>
> Thanks for an update, my comments below.
Thank you for the review. I have started to review my code based on your comments and will get back to you soon.
Thank You.
Regards,
Kumar
On Tue, 4 Oct 2022, [email protected] wrote:
> > -----Original Message-----
> > From: Ilpo J?rvinen <[email protected]>
> > Sent: Monday, October 3, 2022 2:51 PM
> > To: Kumaravel Thiagarajan - I21417 <[email protected]>
> > Subject: Re: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485
> > support to quad-uart driver.
> >
> > On Sat, 1 Oct 2022, Kumaravel Thiagarajan wrote:
> >
> > > pci1xxxx uart supports rs485 mode of operation in the hardware with
> > > auto-direction control with configurable delay for releasing RTS after
> > > the transmission. This patch adds support for the rs485 mode.
> > >
> > > Signed-off-by: Kumaravel Thiagarajan
> > > <[email protected]>
> > > ---
> > > Changes in v2:
> > > - move pci1xxxx_rs485_config to a separate patch with
> > > pci1xxxx_rs485_supported.
> > > ---
> > > drivers/tty/serial/8250/8250_pci1xxxx.c | 57
> > > +++++++++++++++++++++++++
> > > 1 file changed, 57 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > > b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > > index 41a4b94f52b4..999e5a284266 100644
> > > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > > +
> > > + if (rs485->delay_rts_after_send) {
> > > + baud_period_in_ns = ((clock_div >> 8) * 16);
> >
> > Is this 16 perhaps UART_BIT_SAMPLE_CNT?
> Yes. Is there any macro definition for that? I could not find any
> definition in the above name.
You're adding it in your 1/3 patch :-).
--
i.
On Mon, 2022-10-03 at 12:22 +0300, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> > + return 0;
> > +}
>
> Do you really have cards that are providing IO ports? If not, simplify
> this accordingly.
Device does not have IO ports. I will remove support in code.
> > + if (num_vectors == 4)
>
> This check should take care of all possible MSI >= 2, correct?
Hardware supports 2 modes:
1. One irq vector for all the instances
2. nth irq vector for nth instance
Some subsystem ID like PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p and
PCI_SUBDEVICE_ID_MCHP_PCI11414 will fail in 2nd mode if all the 4 irq vectors
are not assigned. Therefore, to reduce complexity, software will handle only 1
or 4 irq vectors.
> > + for (i = 0; i < nr_ports; i++) {
> > + if (num_vectors == 4)
>
> Ditto.
Hardware supports 2 modes:
1. One irq vector for all the instances
2. nth irq vector for nth instance
Some subsystem ID like PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p and
PCI_SUBDEVICE_ID_MCHP_PCI11414 will fail in 2nd mode if all the 4 irq vectors
are not assigned. Therefore, to reduce complexity, software will handle only 1
or 4 irq vectors.
> > + .flags = UART_CAP_FIFO,
> > + },
>
> Can you assign this in ->setup() or so instead of adding a new port type?
Okay, I will assign in pci1xxxx_setup API.
Thanks Andy for the review. Going forward, I will be addressing the comments (if
any).
Thanks,
Tharun Kumar P
On Mon, 2022-10-03 at 12:26 +0300, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> > + port->suspended == 0
>
> How is this check race-protected?
I will use mutex_lock to handle race condition.
Thanks,
Tharun Kumar P
On Mon, 2022-10-03 at 21:36 +0200, Christophe JAILLET wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> > + }
> > + priv->line[i] = serial8250_register_8250_port(&uart);
>
> In case of error, this should be undone in an error handling path in the
> probe, as done in the remove() function below.
>
> If we break, we still continue and return success. But the last
> priv->line[i] are still 0. Is it an issue when pci1xxxx_serial_remove()
> is called?
This will not cause a problem in pci1xxxx_serial_remove as this condition 'priv-
>line[i] >= 0' will be checked for each of the ports before calling
serial8250_unregister_port API.
Thanks,
Tharun Kumar P
Le 26/10/2022 à 13:12, [email protected] a écrit :
> On Mon, 2022-10-03 at 21:36 +0200, Christophe JAILLET wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>> content is safe
>>> + }
>>> + priv->line[i] = serial8250_register_8250_port(&uart);
>>
>> In case of error, this should be undone in an error handling path in the
>> probe, as done in the remove() function below.
>>
>> If we break, we still continue and return success. But the last
>> priv->line[i] are still 0. Is it an issue when pci1xxxx_serial_remove()
>> is called?
>
> This will not cause a problem in pci1xxxx_serial_remove as this condition 'priv-
>> line[i] >= 0' will be checked for each of the ports before calling
> serial8250_unregister_port API.
Yes, this is my point.
We check for >=0 in pci1xxxx_serial_remove(), but if we have an error in
the "for (i = 0; i < nr_ports; i++)" loop, some line[i] we'll still be
zeroed because 'priv' is zalloc'ed.
In such a case, the probe still succeeds.
So, if pci1xxxx_serial_remove() is called later, we potentially call
serial8250_unregister_port(0) several times.
This can lead to double (or more) free in serial8250_em485_destroy()
(but maybe this path can't be taken here?) or maybe some other troubles
elsewhere (I've not checked all the logic in uart_remove_one_port() to
make sure that calling several times this function with the same args is
fine).
So my point is maybe just a "can't happen" case.
If so, apologize for the noise.
CJ
>
>
> Thanks,
> Tharun Kumar P
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Monday, October 3, 2022 2:53 PM
> To: Kumaravel Thiagarajan - I21417
> <[email protected]>
> Subject: Re: [PATCH v2 tty-next 1/3] 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
>
> > + [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,
> > + },
>
> Can you assign this in ->setup() or so instead of adding a new port type?
Hi Andy,
If I understand correctly, you suggest doing something like this inside pci1xxxx_setup() API:
pci1xxxx_setup(.., uart_8250_port *port, ..) {
..
port->port.fifosize = 256;
port->tx_loadsz = 256;
port->capabilities = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01;
..
}
instead of adding new port type PORT_MCHP16550A. But, if I do this, I cannot use sysfs interface for updating rx_trig_bytes right?
Kindly correct me if my understanding is wrong.
Thanks,
Tharun Kumar P
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Monday, October 3, 2022 2:57 PM
> To: Kumaravel Thiagarajan - I21417
> <[email protected]>
> Subject: Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power
> management functions to quad-uart driver.
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> If you have similarities with 8250_pci, probably you need to split it to
> 8250_pcilib.c and share. (See how 8250_dw /8250_lpss are done in that
> sense.)
Functions related to suspend and resume use registers specific to our IP which other drivers cannot use. I will move functions like pci1xxxx_set_divisor, pci1xxxx_get_divisor, pci1xxxx_setup_port and pci1xxxx_rs485_config to a new file.
Thanks,
Tharun Kumar P
On Mon, Oct 31, 2022 at 11:24 AM <[email protected]> wrote:
> > -----Original Message-----
> > From: Andy Shevchenko <[email protected]>
> > Sent: Monday, October 3, 2022 2:53 PM
...
> > > + [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,
> > > + },
> >
> > Can you assign this in ->setup() or so instead of adding a new port type?
>
> If I understand correctly, you suggest doing something like this inside pci1xxxx_setup() API:
>
> pci1xxxx_setup(.., uart_8250_port *port, ..) {
> ..
> port->port.fifosize = 256;
> port->tx_loadsz = 256;
> port->capabilities = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01;
> ..
> }
>
> instead of adding new port type PORT_MCHP16550A.
Yes, something like this.
> But, if I do this, I cannot use sysfs interface for updating rx_trig_bytes right?
Maybe, I don't remember by heart that part of the code. But why do you
need that ABI in the first place?
--
With Best Regards,
Andy Shevchenko
> From: Andy Shevchenko <[email protected]>
> Sent: Monday, October 31, 2022 8:07 PM
> To: Tharunkumar Pasumarthi - I67821
> <[email protected]>
> Subject: Re: [PATCH v2 tty-next 1/3] 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
>
> > But, if I do this, I cannot use sysfs interface for updating rx_trig_bytes right?
>
> Maybe, I don't remember by heart that part of the code. But why do you
> need that ABI in the first place?
By using the sysfs interface, our driver will be able to update the trigger level for the receiver fifo interrupt at runtime.
Thanks,
Tharun Kumar P
On Tue, 1 Nov 2022, [email protected] wrote:
> > From: Ilpo J?rvinen <[email protected]>
> > Sent: Monday, October 3, 2022 2:34 PM
> > To: Kumaravel Thiagarajan - I21417
> > <[email protected]>
> > Subject: Re: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485
> > support to quad-uart driver.
> >
> > [Some people who received this message don't often get email from
> > [email protected]. Learn why this is important at
> > https://aka.ms/LearnAboutSenderIdentification ]
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > > + if (rs485->flags & SER_RS485_ENABLED) {
> > > + memset(rs485->padding, 0, sizeof(rs485->padding));
> >
> > Core handles this for you.
>
> I went through the code and it seems like this is not taken care by the core.
> Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
> This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.
>
> > > + if (!(rs485->flags & SER_RS485_RTS_ON_SEND)) {
> > > + data |= ADCL_CFG_POL_SEL;
> > > + rs485->flags |= SER_RS485_RTS_AFTER_SEND;
> > > + } else {
> > > + rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
> > > + }
> >
> > Core handles that flags sanitization for you.
>
> I went through the code and it seems like this is not taken care by the core.
> Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
> This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.
>
> > > + } else {
> > > + memset(rs485, 0, sizeof(*rs485));
> >
> > Core handles this.
>
> I went through the code and it seems like this is not taken care by the core.
> Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
> This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.
>
> > > + writeb(data, (port->membase + ADCL_CFG_REG));
> > > + port->rs485 = *rs485;
> >
> > Core handles this.
>
> I went through the code and it seems like this is not taken care by the core.
> Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
> This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.
It has nothing to do with serial8250_em485_config.
It is very hard to believe you couldn't find
uart_sanitize_serial_rs485() and uart_set_rs485_config() yourself, the
latter calls your driver specific rs485 handler.
--
i.
> From: Ilpo J?rvinen <[email protected]>
> Sent: Monday, October 3, 2022 2:34 PM
> To: Kumaravel Thiagarajan - I21417
> <[email protected]>
> Subject: Re: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485
> support to quad-uart driver.
>
> [Some people who received this message don't often get email from
> [email protected]. Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> > + if (rs485->flags & SER_RS485_ENABLED) {
> > + memset(rs485->padding, 0, sizeof(rs485->padding));
>
> Core handles this for you.
I went through the code and it seems like this is not taken care by the core.
Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.
> > + if (!(rs485->flags & SER_RS485_RTS_ON_SEND)) {
> > + data |= ADCL_CFG_POL_SEL;
> > + rs485->flags |= SER_RS485_RTS_AFTER_SEND;
> > + } else {
> > + rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
> > + }
>
> Core handles that flags sanitization for you.
I went through the code and it seems like this is not taken care by the core.
Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.
> > + } else {
> > + memset(rs485, 0, sizeof(*rs485));
>
> Core handles this.
I went through the code and it seems like this is not taken care by the core.
Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.
> > + writeb(data, (port->membase + ADCL_CFG_REG));
> > + port->rs485 = *rs485;
>
> Core handles this.
I went through the code and it seems like this is not taken care by the core.
Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.
Thanks,
Tharun Kumar P
On Tue, 1 Nov 2022, Andy Shevchenko wrote:
> On Tue, Nov 1, 2022 at 5:25 PM Ilpo J?rvinen
> <[email protected]> wrote:
> > On Tue, 1 Nov 2022, [email protected] wrote:
>
> ...
>
> > > I went through the code and it seems like this is not taken care by the core.
> > > Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
> > > This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.
> >
> > It has nothing to do with serial8250_em485_config.
> >
> > It is very hard to believe you couldn't find
> > uart_sanitize_serial_rs485() and uart_set_rs485_config() yourself, the
> > latter calls your driver specific rs485 handler.
>
> Which version has this API? If it's v6.1-rc1 and patches are made
> against v6.0, it's possible to miss something.
>
> In any case, the patches to the serial subsystem should always be done
> against the tty/tty-next branch.
It has been for multiple stable kernel version already. It was moved to
own function by this commit:
git describe --contains 2dbd0c14ebe8836eaf890c7f50f3fc5d26d67d95
v6.0-rc1~64^2~129
Originally introduced here:
git describe --contains 0ed12afa5655512ee418047fb3546d229df20aa1
v5.19-rc1~47^2~142
There have been perhaps one of those things I pointed out that was added
later than the others but it won't explain why nothing was found from the
code.
--
i.
On Tue, Nov 1, 2022 at 5:04 PM <[email protected]> wrote:
> > From: Andy Shevchenko <[email protected]>
> > Sent: Monday, October 31, 2022 8:07 PM
...
> > > But, if I do this, I cannot use sysfs interface for updating rx_trig_bytes right?
> >
> > Maybe, I don't remember by heart that part of the code. But why do you
> > need that ABI in the first place?
>
> By using the sysfs interface, our driver will be able to update the trigger level for the receiver fifo interrupt at runtime.
This doesn't answer my question. What is this needed for?
--
With Best Regards,
Andy Shevchenko
On Tue, Nov 1, 2022 at 5:25 PM Ilpo Järvinen
<[email protected]> wrote:
> On Tue, 1 Nov 2022, [email protected] wrote:
...
> > I went through the code and it seems like this is not taken care by the core.
> > Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
> > This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.
>
> It has nothing to do with serial8250_em485_config.
>
> It is very hard to believe you couldn't find
> uart_sanitize_serial_rs485() and uart_set_rs485_config() yourself, the
> latter calls your driver specific rs485 handler.
Which version has this API? If it's v6.1-rc1 and patches are made
against v6.0, it's possible to miss something.
In any case, the patches to the serial subsystem should always be done
against the tty/tty-next branch.
--
With Best Regards,
Andy Shevchenko
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Tuesday, November 1, 2022 8:47 PM
> To: Tharunkumar Pasumarthi - I67821
> <[email protected]>
> Subject: Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for
> quad-uart support.
>
> On Tue, Nov 1, 2022 at 5:04 PM <[email protected]>
> wrote:
> > > From: Andy Shevchenko <[email protected]>
> > > Sent: Monday, October 31, 2022 8:07 PM
>
> ...
>
> > > > But, if I do this, I cannot use sysfs interface for updating rx_trig_bytes
> right?
> > >
> > > Maybe, I don't remember by heart that part of the code. But why do
> > > you need that ABI in the first place?
> >
> > By using the sysfs interface, our driver will be able to update the trigger
> level for the receiver fifo interrupt at runtime.
>
> This doesn't answer my question. What is this needed for?
I think this will be useful for our customers in tuning the trigger level based on their application.
This is a UART based on programmed I/O and at higher speeds sometimes where there can be
continuous stream of data for a long time, we may need to set the trigger to the lower side to detect
early and avoid overflow.
On the other hand, in some applications where the continuous stream of data is not very long and
no risk of overflow, set it to higher side to avoid frequent interrupts..
We just want to keep this option open in the driver for different customers who would want to tune
this based on their application.
Please let us know if there are further questions or any issues in this approach.
Thank You.
Regards,
Kumar
> From: Ilpo J?rvinen <[email protected]>
> Sent: Tuesday, November 1, 2022 9:20 PM
> To: Andy Shevchenko <[email protected]>
> Subject: Re: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485
> support to quad-uart driver.
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> There have been perhaps one of those things I pointed out that was added
> later than the others but it won't explain why nothing was found from the
> code.
Sorry Ilpo. This was a mistake from my side. I was referring to older kernel instead of the one in tty-next branch by mistake. I will make sure not to repeat this going forward.
Thanks,
Tharun Kumar P
> From: Andy Shevchenko <[email protected]>
> Sent: Monday, October 3, 2022 2:57 PM
> To: Kumaravel Thiagarajan - I21417
> <[email protected]>
> Subject: Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power
> management functions to quad-uart driver.
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> If you have similarities with 8250_pci, probably you need to split it to
> 8250_pcilib.c and share. (See how 8250_dw /8250_lpss are done in that
> sense.)
Hi Andy,
All the functions used in 8250_pci1xxxx.c that have similarity with 8250_pci use registers
that are specific to our IP. The only function that can be moved to common library is the
setup_port. But, for that the first argument of setup_port must be changed to
'struct pci_dev *dev' (priv->dev). Do you suggest doing this?
Thanks,
Tharun Kumar P
On Fri, Nov 4, 2022 at 12:23 PM <[email protected]> wrote:
> > From: Andy Shevchenko <[email protected]>
> > Sent: Monday, October 3, 2022 2:57 PM
> > If you have similarities with 8250_pci, probably you need to split it to
> > 8250_pcilib.c and share. (See how 8250_dw /8250_lpss are done in that
> > sense.)
>
> All the functions used in 8250_pci1xxxx.c that have similarity with 8250_pci use registers
> that are specific to our IP. The only function that can be moved to common library is the
> setup_port. But, for that the first argument of setup_port must be changed to
> 'struct pci_dev *dev' (priv->dev). Do you suggest doing this?
So, you can create a common serial8250_setup_port(struct pci_dev *dev,
...) and call it from the static setup_port() inside 8250_pci.c. This
way you won't introduce too many churn.
--
With Best Regards,
Andy Shevchenko