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.
Kumaravel Thiagarajan (2):
8250: microchip: pci1xxxx: Add driver for the quad-uart function in
the multi-function endpoint of pci1xxxx device.
8250: microchip: pci1xxxx: Add power management functions to
pci1xxxx's quad-uart driver.
MAINTAINERS | 6 +
drivers/tty/serial/8250/8250_pci1xxxx.c | 585 ++++++++++++++++++++++++
drivers/tty/serial/8250/8250_port.c | 8 +
drivers/tty/serial/8250/Kconfig | 9 +
drivers/tty/serial/8250/Makefile | 1 +
include/uapi/linux/serial_core.h | 3 +
6 files changed, 612 insertions(+)
create mode 100644 drivers/tty/serial/8250/8250_pci1xxxx.c
--
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]>
---
MAINTAINERS | 6 +
drivers/tty/serial/8250/8250_pci1xxxx.c | 463 ++++++++++++++++++++++++
drivers/tty/serial/8250/8250_port.c | 8 +
drivers/tty/serial/8250/Kconfig | 9 +
drivers/tty/serial/8250/Makefile | 1 +
include/uapi/linux/serial_core.h | 3 +
6 files changed, 490 insertions(+)
create mode 100644 drivers/tty/serial/8250/8250_pci1xxxx.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 8a5012ba6ff9..b2021425ce08 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..56852ae0585e
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
@@ -0,0 +1,463 @@
+// 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/module.h>
+#include <linux/pci.h>
+#include <linux/string.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/tty.h>
+#include <linux/serial_reg.h>
+#include <linux/serial_core.h>
+#include <linux/8250_pci.h>
+#include <linux/serial_8250.h>
+#include <linux/bitops.h>
+#include <linux/io.h>
+#include <asm/byteorder.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_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_BIT_SAMPLE_CNT 16
+
+struct pci1xxxx_8250 {
+ struct pci_dev *dev;
+ unsigned int nr;
+ void __iomem *membase;
+ int line[];
+};
+
+static int setup_port(struct pci1xxxx_8250 *priv, struct uart_8250_port *port,
+ int bar, 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;
+}
+
+static int mchp_pci1xxxx_rs485_config(struct uart_port *port,
+ struct ktermios *termios,
+ struct serial_rs485 *rs485)
+{
+ u8 data = 0;
+
+ memset(rs485->padding, 0, sizeof(rs485->padding));
+ rs485->flags &= SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND;
+
+ if (rs485->flags & SER_RS485_ENABLED) {
+ 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;
+ }
+ }
+
+ rs485->delay_rts_after_send = 0;
+ rs485->delay_rts_before_send = 0;
+ writeb(data, (port->membase + ADCL_CFG_REG));
+ port->rs485 = *rs485;
+
+ return 0;
+}
+
+static void mchp_pci1xxxx_set_termios(struct uart_port *port,
+ struct ktermios *termios,
+ struct ktermios *old)
+{
+ const unsigned int standard_baud_list[] = {50, 75, 110, 134, 150, 300,
+ 600, 1200, 1800, 2000, 2400, 3600,
+ 4800, 7200, 9600, 19200, 38400, 57600,
+ 115200, 125000, 136400, 150000, 166700,
+ 187500, 214300, 250000, 300000, 375000,
+ 500000, 750000, 1000000, 1500000};
+ unsigned int baud = tty_termios_baud_rate(termios);
+ unsigned int baud_clock;
+ unsigned int quot;
+ unsigned int frac;
+ unsigned int i;
+
+ baud = tty_termios_baud_rate(termios);
+ serial8250_do_set_termios(port, termios, NULL);
+
+ if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST) {
+ writel((port->custom_divisor & 0x3FFFFFFF),
+ (port->membase + CLK_DIVISOR_REG));
+ } else {
+ for (i = 0; i < ARRAY_SIZE(standard_baud_list); i++) {
+ if (baud == standard_baud_list[i])
+ return;
+ }
+ tty_termios_encode_baud_rate(termios, baud, baud);
+
+ baud = uart_get_baud_rate(port, termios, old,
+ 50, 1500000);
+ baud_clock = readb(port->membase + CLK_SEL_REG);
+
+ if ((baud_clock & CLK_SEL_MASK) == CLK_SEL_500MHZ) {
+ quot = 500000000 / (16 * baud);
+ writel(quot, (port->membase + CLK_DIVISOR_REG));
+ } else if ((baud_clock & CLK_SEL_MASK) == CLK_SEL_166MHZ) {
+ quot = (166667 * 1000) / (16 * baud);
+ writel(quot, (port->membase + CLK_DIVISOR_REG));
+ } else {
+ quot = ((1000000000) / (baud * UART_BIT_SAMPLE_CNT));
+ frac = (((1000000000 - (quot * baud *
+ UART_BIT_SAMPLE_CNT)) / UART_BIT_SAMPLE_CNT)
+ * 255) / baud;
+ writel(frac | (quot << 8),
+ (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 mchp_pci1xxxx_setup(struct pci1xxxx_8250 *priv,
+ struct uart_8250_port *port, int idx)
+{
+ int first_offset = 0;
+ int offset;
+ u8 *data;
+ int ret;
+
+ switch (priv->dev->subsystem_device) {
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1:
+ first_offset = 256;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2:
+ first_offset = 512;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3:
+ first_offset = 768;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02:
+ if (idx > 0)
+ idx++;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03:
+ if (idx > 0)
+ idx += 2;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12:
+ first_offset = 256;
+ if (idx > 0)
+ idx++;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13:
+ first_offset = 256;
+ if (idx > 0)
+ idx++;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23:
+ first_offset = 512;
+ break;
+
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123:
+ first_offset = 256;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013:
+ if (idx > 1)
+ idx++;
+ break;
+ case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023:
+ if (idx > 0)
+ idx++;
+ break;
+ }
+
+ data = devm_kzalloc(&priv->dev->dev, sizeof(u8), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ offset = first_offset + (idx * 256);
+ port->port.flags |= UPF_FIXED_TYPE | UPF_SKIP_TEST;
+ port->port.type = PORT_MCHP16550A;
+ port->port.rs485_config = mchp_pci1xxxx_rs485_config;
+ port->port.set_termios = mchp_pci1xxxx_set_termios;
+ ret = setup_port(priv, port, 0x00, offset, 0x00);
+ 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;
+}
+
+void mchp_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);
+ pci_save_state(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);
+
+ priv->membase = pcim_iomap(dev, 0, 0);
+ priv->dev = dev;
+ priv->nr = nr_ports;
+
+ if (!priv)
+ return -ENOMEM;
+
+ pci_set_master(dev);
+
+ num_vectors = pci_alloc_irq_vectors(dev, 1, 4, PCI_IRQ_ALL_TYPES);
+ if (num_vectors < 0)
+ return rc;
+
+ memset(&uart, 0, sizeof(uart));
+ uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_TYPE | UPF_FIXED_PORT;
+ uart.port.uartclk = 48000000;
+ 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)
+ mchp_pci1xxxx_irq_assign(priv, &uart, i);
+ rc = mchp_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++)
+ 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) },
+ {0,}
+};
+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 39b35a61958c..64ba32837838 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..d4b1344d0628 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -528,6 +528,15 @@ 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
+ 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..2bd82ac0ae1a 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -276,4 +276,7 @@
/* Sunplus UART */
#define PORT_SUNPLUS 123
+/* MCHP 16550A UART with 256 byte FIFOs */
+#define PORT_MCHP16550A 124
+
#endif /* _UAPILINUX_SERIAL_CORE_H */
--
2.25.1
On Tue, Aug 30, 2022 at 9:01 PM 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 the contribution!
Brief looking into the code I can see that you may easily reduce it by ~20%.
Think about it. You may take other examples, that are servicing PCI based
devices (8250_exar, 8250_lpss, 8250_mid) on how to shrink the code base.
...
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/string.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/tty.h>
> +#include <linux/serial_reg.h>
> +#include <linux/serial_core.h>
> +#include <linux/8250_pci.h>
> +#include <linux/serial_8250.h>
> +#include <linux/bitops.h>
> +#include <linux/io.h>
Why not sorted?
Do you need all of them?
...
> + const unsigned int standard_baud_list[] = {50, 75, 110, 134, 150, 300,
> + 600, 1200, 1800, 2000, 2400, 3600,
> + 4800, 7200, 9600, 19200, 38400, 57600,
> + 115200, 125000, 136400, 150000, 166700,
> + 187500, 214300, 250000, 300000, 375000,
> + 500000, 750000, 1000000, 1500000};
Why?!
...
> + if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST) {
No. We don't want to have this in the new drivers. There is BOTHER
which might be used instead.
> + writel((port->custom_divisor & 0x3FFFFFFF),
> + (port->membase + CLK_DIVISOR_REG));
...
> + frac = (((1000000000 - (quot * baud *
> + UART_BIT_SAMPLE_CNT)) / UART_BIT_SAMPLE_CNT)
> + * 255) / baud;
Funny indentation.
...
> +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);
> + pci_save_state(dev);
Why is this call here?
> + if (rc)
> + return rc;
> +
> + nr_ports = pci1xxxx_get_num_ports(dev);
> +
> + priv = devm_kzalloc(&dev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
> +
> + priv->membase = pcim_iomap(dev, 0, 0);
> + priv->dev = dev;
> + priv->nr = nr_ports;
> + if (!priv)
> + return -ENOMEM;
You are dereferencing a NULL pointer before checking, how did you test
your code?
> + pci_set_master(dev);
> +
> + num_vectors = pci_alloc_irq_vectors(dev, 1, 4, PCI_IRQ_ALL_TYPES);
> + if (num_vectors < 0)
> + return rc;
What does this mean?
> + memset(&uart, 0, sizeof(uart));
> + uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_TYPE | UPF_FIXED_PORT;
> + uart.port.uartclk = 48000000;
> + 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)
> + mchp_pci1xxxx_irq_assign(priv, &uart, i);
> + rc = mchp_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 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) },
> + {0,}
{ } is enough
> +};
...
> +
Unneeded blank line
> +module_pci_driver(pci1xxxx_pci_driver);
...
> + [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,
> + },
Why do you need this?
...
> +/* MCHP 16550A UART with 256 byte FIFOs */
> +#define PORT_MCHP16550A 124
...and this?
If you really need this, try to find a gap in the numbering, there are some.
--
With Best Regards,
Andy Shevchenko
Hi Kumaravel,
On Tue, Aug 30, 2022 at 8:01 PM 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.
>
> Signed-off-by: Kumaravel Thiagarajan <[email protected]>
Thanks for your patch!
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> +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);
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -528,6 +528,15 @@ 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
As this is a PCI driver, I guess it should depend on PCI
(|| COMPILE_TEST)?
> + 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.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Kumaravel,
I love your patch! Yet something to improve:
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on linus/master v6.0-rc3 next-20220830]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kumaravel-Thiagarajan/8250-microchip-pci1xxxx-Add-driver-for-the-pci1xxxx-s-quad-uart-function/20220831-020314
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20220831/[email protected]/config)
compiler: sh4-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/075ee716bd7ce075396d0539dffa4ae59e6b985a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kumaravel-Thiagarajan/8250-microchip-pci1xxxx-Add-driver-for-the-pci1xxxx-s-quad-uart-function/20220831-020314
git checkout 075ee716bd7ce075396d0539dffa4ae59e6b985a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash drivers/tty/serial/8250/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/tty/serial/8250/8250_pci1xxxx.c: In function 'mchp_pci1xxxx_setup':
>> drivers/tty/serial/8250/8250_pci1xxxx.c:289:32: error: assignment to 'void (*)(struct uart_port *, struct ktermios *, const struct ktermios *)' from incompatible pointer type 'void (*)(struct uart_port *, struct ktermios *, struct ktermios *)' [-Werror=incompatible-pointer-types]
289 | port->port.set_termios = mchp_pci1xxxx_set_termios;
| ^
drivers/tty/serial/8250/8250_pci1xxxx.c: At top level:
drivers/tty/serial/8250/8250_pci1xxxx.c:301:6: warning: no previous prototype for 'mchp_pci1xxxx_irq_assign' [-Wmissing-prototypes]
301 | void mchp_pci1xxxx_irq_assign(struct pci1xxxx_8250 *priv,
| ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/tty/serial/8250/8250_pci1xxxx.c: In function 'pci1xxxx_serial_probe':
>> drivers/tty/serial/8250/8250_pci1xxxx.c:395:57: error: 'PCI_IRQ_ALL_TYPES' undeclared (first use in this function)
395 | num_vectors = pci_alloc_irq_vectors(dev, 1, 4, PCI_IRQ_ALL_TYPES);
| ^~~~~~~~~~~~~~~~~
drivers/tty/serial/8250/8250_pci1xxxx.c:395:57: note: each undeclared identifier is reported only once for each function it appears in
drivers/tty/serial/8250/8250_pci1xxxx.c: At top level:
drivers/tty/serial/8250/8250_pci1xxxx.c:459:1: warning: data definition has no type or storage class
459 | module_pci_driver(pci1xxxx_pci_driver);
| ^~~~~~~~~~~~~~~~~
>> drivers/tty/serial/8250/8250_pci1xxxx.c:459:1: error: type defaults to 'int' in declaration of 'module_pci_driver' [-Werror=implicit-int]
drivers/tty/serial/8250/8250_pci1xxxx.c:459:1: warning: parameter names (without types) in function declaration
drivers/tty/serial/8250/8250_pci1xxxx.c:452:26: warning: 'pci1xxxx_pci_driver' defined but not used [-Wunused-variable]
452 | static struct pci_driver pci1xxxx_pci_driver = {
| ^~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +289 drivers/tty/serial/8250/8250_pci1xxxx.c
227
228 static int mchp_pci1xxxx_setup(struct pci1xxxx_8250 *priv,
229 struct uart_8250_port *port, int idx)
230 {
231 int first_offset = 0;
232 int offset;
233 u8 *data;
234 int ret;
235
236 switch (priv->dev->subsystem_device) {
237 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1:
238 first_offset = 256;
239 break;
240 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2:
241 first_offset = 512;
242 break;
243 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3:
244 first_offset = 768;
245 break;
246 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02:
247 if (idx > 0)
248 idx++;
249 break;
250 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03:
251 if (idx > 0)
252 idx += 2;
253 break;
254 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12:
255 first_offset = 256;
256 if (idx > 0)
257 idx++;
258 break;
259 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13:
260 first_offset = 256;
261 if (idx > 0)
262 idx++;
263 break;
264 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23:
265 first_offset = 512;
266 break;
267
268 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123:
269 first_offset = 256;
270 break;
271 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013:
272 if (idx > 1)
273 idx++;
274 break;
275 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023:
276 if (idx > 0)
277 idx++;
278 break;
279 }
280
281 data = devm_kzalloc(&priv->dev->dev, sizeof(u8), GFP_KERNEL);
282 if (!data)
283 return -ENOMEM;
284
285 offset = first_offset + (idx * 256);
286 port->port.flags |= UPF_FIXED_TYPE | UPF_SKIP_TEST;
287 port->port.type = PORT_MCHP16550A;
288 port->port.rs485_config = mchp_pci1xxxx_rs485_config;
> 289 port->port.set_termios = mchp_pci1xxxx_set_termios;
290 ret = setup_port(priv, port, 0x00, offset, 0x00);
291 if (ret < 0)
292 return ret;
293
294 writeb(UART_ACTV_SET_ACTIVE, port->port.membase + UART_ACTV_REG);
295 writeb(UART_WAKE_SRCS, port->port.membase + UART_WAKE_REG);
296 writeb(UART_WAKE_N_PIN, port->port.membase + UART_WAKE_MASK_REG);
297
298 return 0;
299 }
300
301 void mchp_pci1xxxx_irq_assign(struct pci1xxxx_8250 *priv,
302 struct uart_8250_port *uart, int idx)
303 {
304 switch (priv->dev->subsystem_device) {
305 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p0:
306 case PCI_SUBDEVICE_ID_MCHP_PCI12000:
307 case PCI_SUBDEVICE_ID_MCHP_PCI11010:
308 case PCI_SUBDEVICE_ID_MCHP_PCI11101:
309 case PCI_SUBDEVICE_ID_MCHP_PCI11400:
310 uart->port.irq = pci_irq_vector(priv->dev, 0);
311 break;
312 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1:
313 uart->port.irq = pci_irq_vector(priv->dev, 1);
314 break;
315 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2:
316 uart->port.irq = pci_irq_vector(priv->dev, 2);
317 break;
318 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3:
319 uart->port.irq = pci_irq_vector(priv->dev, 3);
320 break;
321 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p01:
322 uart->port.irq = pci_irq_vector(priv->dev, idx);
323 break;
324 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02:
325 if (idx > 0)
326 idx++;
327 uart->port.irq = pci_irq_vector(priv->dev, idx);
328 break;
329 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03:
330 if (idx > 0)
331 idx += 2;
332 uart->port.irq = pci_irq_vector(priv->dev, idx);
333 break;
334 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12:
335 uart->port.irq = pci_irq_vector(priv->dev, idx + 1);
336 break;
337 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13:
338 if (idx > 0)
339 idx += 1;
340 uart->port.irq = pci_irq_vector(priv->dev, idx + 1);
341 break;
342 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23:
343 uart->port.irq = pci_irq_vector(priv->dev, idx + 2);
344 break;
345 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p012:
346 uart->port.irq = pci_irq_vector(priv->dev, idx);
347 break;
348 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013:
349 if (idx > 1)
350 idx++;
351 uart->port.irq = pci_irq_vector(priv->dev, idx);
352 break;
353 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023:
354 if (idx > 0)
355 idx++;
356 uart->port.irq = pci_irq_vector(priv->dev, idx);
357 break;
358 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123:
359 uart->port.irq = pci_irq_vector(priv->dev, idx + 1);
360 break;
361 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p:
362 case PCI_SUBDEVICE_ID_MCHP_PCI11414:
363 uart->port.irq = pci_irq_vector(priv->dev, idx);
364 break;
365 }
366 }
367
368 static int pci1xxxx_serial_probe(struct pci_dev *dev,
369 const struct pci_device_id *ent)
370 {
371 struct pci1xxxx_8250 *priv;
372 struct uart_8250_port uart;
373 unsigned int nr_ports, i;
374 int num_vectors = 0;
375 int rc;
376
377 rc = pcim_enable_device(dev);
378 pci_save_state(dev);
379 if (rc)
380 return rc;
381
382 nr_ports = pci1xxxx_get_num_ports(dev);
383
384 priv = devm_kzalloc(&dev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
385
386 priv->membase = pcim_iomap(dev, 0, 0);
387 priv->dev = dev;
388 priv->nr = nr_ports;
389
390 if (!priv)
391 return -ENOMEM;
392
393 pci_set_master(dev);
394
> 395 num_vectors = pci_alloc_irq_vectors(dev, 1, 4, PCI_IRQ_ALL_TYPES);
396 if (num_vectors < 0)
397 return rc;
398
399 memset(&uart, 0, sizeof(uart));
400 uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_TYPE | UPF_FIXED_PORT;
401 uart.port.uartclk = 48000000;
402 uart.port.dev = &dev->dev;
403
404 if (num_vectors == 4)
405 writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI, (priv->membase + UART_PCI_CTRL_REG));
406 else
407 uart.port.irq = pci_irq_vector(dev, 0);
408
409 for (i = 0; i < nr_ports; i++) {
410 if (num_vectors == 4)
411 mchp_pci1xxxx_irq_assign(priv, &uart, i);
412 rc = mchp_pci1xxxx_setup(priv, &uart, i);
413 if (rc) {
414 dev_err(&dev->dev, "Failed to setup port %u\n", i);
415 break;
416 }
417 priv->line[i] = serial8250_register_8250_port(&uart);
418
419 if (priv->line[i] < 0) {
420 dev_err(&dev->dev,
421 "Couldn't register serial port %lx, irq %d, type %d, error %d\n",
422 uart.port.iobase, uart.port.irq,
423 uart.port.iotype, priv->line[i]);
424 break;
425 }
426 }
427
428 pci_set_drvdata(dev, priv);
429
430 return 0;
431 }
432
433 static void pci1xxxx_serial_remove(struct pci_dev *dev)
434 {
435 struct pci1xxxx_8250 *priv = pci_get_drvdata(dev);
436 int i;
437
438 for (i = 0; i < priv->nr; i++)
439 serial8250_unregister_port(priv->line[i]);
440 }
441
442 static const struct pci_device_id pci1xxxx_pci_tbl[] = {
443 { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11010) },
444 { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11101) },
445 { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11400) },
446 { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11414) },
447 { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI12000) },
448 {0,}
449 };
450 MODULE_DEVICE_TABLE(pci, pci1xxxx_pci_tbl);
451
452 static struct pci_driver pci1xxxx_pci_driver = {
453 .name = "pci1xxxx serial",
454 .probe = pci1xxxx_serial_probe,
455 .remove = pci1xxxx_serial_remove,
456 .id_table = pci1xxxx_pci_tbl,
457 };
458
> 459 module_pci_driver(pci1xxxx_pci_driver);
460
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Kumaravel,
I love your patch! Perhaps something to improve:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on linus/master v6.0-rc3 next-20220830]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kumaravel-Thiagarajan/8250-microchip-pci1xxxx-Add-driver-for-the-pci1xxxx-s-quad-uart-function/20220831-020314
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20220831/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/075ee716bd7ce075396d0539dffa4ae59e6b985a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kumaravel-Thiagarajan/8250-microchip-pci1xxxx-Add-driver-for-the-pci1xxxx-s-quad-uart-function/20220831-020314
git checkout 075ee716bd7ce075396d0539dffa4ae59e6b985a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/tty/serial/8250/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/tty/serial/8250/8250_pci1xxxx.c: In function 'mchp_pci1xxxx_setup':
drivers/tty/serial/8250/8250_pci1xxxx.c:289:32: error: assignment to 'void (*)(struct uart_port *, struct ktermios *, const struct ktermios *)' from incompatible pointer type 'void (*)(struct uart_port *, struct ktermios *, struct ktermios *)' [-Werror=incompatible-pointer-types]
289 | port->port.set_termios = mchp_pci1xxxx_set_termios;
| ^
drivers/tty/serial/8250/8250_pci1xxxx.c: At top level:
>> drivers/tty/serial/8250/8250_pci1xxxx.c:301:6: warning: no previous prototype for 'mchp_pci1xxxx_irq_assign' [-Wmissing-prototypes]
301 | void mchp_pci1xxxx_irq_assign(struct pci1xxxx_8250 *priv,
| ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/tty/serial/8250/8250_pci1xxxx.c: In function 'pci1xxxx_serial_probe':
drivers/tty/serial/8250/8250_pci1xxxx.c:395:57: error: 'PCI_IRQ_ALL_TYPES' undeclared (first use in this function)
395 | num_vectors = pci_alloc_irq_vectors(dev, 1, 4, PCI_IRQ_ALL_TYPES);
| ^~~~~~~~~~~~~~~~~
drivers/tty/serial/8250/8250_pci1xxxx.c:395:57: note: each undeclared identifier is reported only once for each function it appears in
drivers/tty/serial/8250/8250_pci1xxxx.c: At top level:
>> drivers/tty/serial/8250/8250_pci1xxxx.c:459:1: warning: data definition has no type or storage class
459 | module_pci_driver(pci1xxxx_pci_driver);
| ^~~~~~~~~~~~~~~~~
drivers/tty/serial/8250/8250_pci1xxxx.c:459:1: error: type defaults to 'int' in declaration of 'module_pci_driver' [-Werror=implicit-int]
>> drivers/tty/serial/8250/8250_pci1xxxx.c:459:1: warning: parameter names (without types) in function declaration
drivers/tty/serial/8250/8250_pci1xxxx.c:452:26: warning: 'pci1xxxx_pci_driver' defined but not used [-Wunused-variable]
452 | static struct pci_driver pci1xxxx_pci_driver = {
| ^~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/mchp_pci1xxxx_irq_assign +301 drivers/tty/serial/8250/8250_pci1xxxx.c
300
> 301 void mchp_pci1xxxx_irq_assign(struct pci1xxxx_8250 *priv,
302 struct uart_8250_port *uart, int idx)
303 {
304 switch (priv->dev->subsystem_device) {
305 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p0:
306 case PCI_SUBDEVICE_ID_MCHP_PCI12000:
307 case PCI_SUBDEVICE_ID_MCHP_PCI11010:
308 case PCI_SUBDEVICE_ID_MCHP_PCI11101:
309 case PCI_SUBDEVICE_ID_MCHP_PCI11400:
310 uart->port.irq = pci_irq_vector(priv->dev, 0);
311 break;
312 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1:
313 uart->port.irq = pci_irq_vector(priv->dev, 1);
314 break;
315 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2:
316 uart->port.irq = pci_irq_vector(priv->dev, 2);
317 break;
318 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3:
319 uart->port.irq = pci_irq_vector(priv->dev, 3);
320 break;
321 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p01:
322 uart->port.irq = pci_irq_vector(priv->dev, idx);
323 break;
324 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02:
325 if (idx > 0)
326 idx++;
327 uart->port.irq = pci_irq_vector(priv->dev, idx);
328 break;
329 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03:
330 if (idx > 0)
331 idx += 2;
332 uart->port.irq = pci_irq_vector(priv->dev, idx);
333 break;
334 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12:
335 uart->port.irq = pci_irq_vector(priv->dev, idx + 1);
336 break;
337 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13:
338 if (idx > 0)
339 idx += 1;
340 uart->port.irq = pci_irq_vector(priv->dev, idx + 1);
341 break;
342 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23:
343 uart->port.irq = pci_irq_vector(priv->dev, idx + 2);
344 break;
345 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p012:
346 uart->port.irq = pci_irq_vector(priv->dev, idx);
347 break;
348 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013:
349 if (idx > 1)
350 idx++;
351 uart->port.irq = pci_irq_vector(priv->dev, idx);
352 break;
353 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023:
354 if (idx > 0)
355 idx++;
356 uart->port.irq = pci_irq_vector(priv->dev, idx);
357 break;
358 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123:
359 uart->port.irq = pci_irq_vector(priv->dev, idx + 1);
360 break;
361 case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p:
362 case PCI_SUBDEVICE_ID_MCHP_PCI11414:
363 uart->port.irq = pci_irq_vector(priv->dev, idx);
364 break;
365 }
366 }
367
368 static int pci1xxxx_serial_probe(struct pci_dev *dev,
369 const struct pci_device_id *ent)
370 {
371 struct pci1xxxx_8250 *priv;
372 struct uart_8250_port uart;
373 unsigned int nr_ports, i;
374 int num_vectors = 0;
375 int rc;
376
377 rc = pcim_enable_device(dev);
378 pci_save_state(dev);
379 if (rc)
380 return rc;
381
382 nr_ports = pci1xxxx_get_num_ports(dev);
383
384 priv = devm_kzalloc(&dev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
385
386 priv->membase = pcim_iomap(dev, 0, 0);
387 priv->dev = dev;
388 priv->nr = nr_ports;
389
390 if (!priv)
391 return -ENOMEM;
392
393 pci_set_master(dev);
394
395 num_vectors = pci_alloc_irq_vectors(dev, 1, 4, PCI_IRQ_ALL_TYPES);
396 if (num_vectors < 0)
397 return rc;
398
399 memset(&uart, 0, sizeof(uart));
400 uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_TYPE | UPF_FIXED_PORT;
401 uart.port.uartclk = 48000000;
402 uart.port.dev = &dev->dev;
403
404 if (num_vectors == 4)
405 writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI, (priv->membase + UART_PCI_CTRL_REG));
406 else
407 uart.port.irq = pci_irq_vector(dev, 0);
408
409 for (i = 0; i < nr_ports; i++) {
410 if (num_vectors == 4)
411 mchp_pci1xxxx_irq_assign(priv, &uart, i);
412 rc = mchp_pci1xxxx_setup(priv, &uart, i);
413 if (rc) {
414 dev_err(&dev->dev, "Failed to setup port %u\n", i);
415 break;
416 }
417 priv->line[i] = serial8250_register_8250_port(&uart);
418
419 if (priv->line[i] < 0) {
420 dev_err(&dev->dev,
421 "Couldn't register serial port %lx, irq %d, type %d, error %d\n",
422 uart.port.iobase, uart.port.irq,
423 uart.port.iotype, priv->line[i]);
424 break;
425 }
426 }
427
428 pci_set_drvdata(dev, priv);
429
430 return 0;
431 }
432
433 static void pci1xxxx_serial_remove(struct pci_dev *dev)
434 {
435 struct pci1xxxx_8250 *priv = pci_get_drvdata(dev);
436 int i;
437
438 for (i = 0; i < priv->nr; i++)
439 serial8250_unregister_port(priv->line[i]);
440 }
441
442 static const struct pci_device_id pci1xxxx_pci_tbl[] = {
443 { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11010) },
444 { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11101) },
445 { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11400) },
446 { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11414) },
447 { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI12000) },
448 {0,}
449 };
450 MODULE_DEVICE_TABLE(pci, pci1xxxx_pci_tbl);
451
452 static struct pci_driver pci1xxxx_pci_driver = {
453 .name = "pci1xxxx serial",
454 .probe = pci1xxxx_serial_probe,
455 .remove = pci1xxxx_serial_remove,
456 .id_table = pci1xxxx_pci_tbl,
457 };
458
> 459 module_pci_driver(pci1xxxx_pci_driver);
460
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Tue, 30 Aug 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]>
> ---
> +static int mchp_pci1xxxx_rs485_config(struct uart_port *port,
> + struct ktermios *termios,
> + struct serial_rs485 *rs485)
> +{
> + u8 data = 0;
> +
> + memset(rs485->padding, 0, sizeof(rs485->padding));
> + rs485->flags &= SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND;
> +
> + if (rs485->flags & SER_RS485_ENABLED) {
> + 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;
> + }
> + }
> +
> + rs485->delay_rts_after_send = 0;
> + rs485->delay_rts_before_send = 0;
> + writeb(data, (port->membase + ADCL_CFG_REG));
> + port->rs485 = *rs485;
Most of this will be handled for by the core so don't duplicate it in
the driver.
These days, you also need to provide rs485_supported.
> + return 0;
> +}
> +
> +static void mchp_pci1xxxx_set_termios(struct uart_port *port,
> + struct ktermios *termios,
> + struct ktermios *old)
> +{
> + const unsigned int standard_baud_list[] = {50, 75, 110, 134, 150, 300,
> + 600, 1200, 1800, 2000, 2400, 3600,
> + 4800, 7200, 9600, 19200, 38400, 57600,
> + 115200, 125000, 136400, 150000, 166700,
> + 187500, 214300, 250000, 300000, 375000,
> + 500000, 750000, 1000000, 1500000};
> + unsigned int baud = tty_termios_baud_rate(termios);
> + unsigned int baud_clock;
> + unsigned int quot;
> + unsigned int frac;
> + unsigned int i;
> +
> + baud = tty_termios_baud_rate(termios);
Either this or the one at the declaration is redundant.
> + serial8250_do_set_termios(port, termios, NULL);
> +
> + if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST) {
> + writel((port->custom_divisor & 0x3FFFFFFF),
> + (port->membase + CLK_DIVISOR_REG));
> + } else {
> + for (i = 0; i < ARRAY_SIZE(standard_baud_list); i++) {
> + if (baud == standard_baud_list[i])
> + return;
Did you mean to break here instead?
> + }
> + tty_termios_encode_baud_rate(termios, baud, baud);
> +
> + baud = uart_get_baud_rate(port, termios, old,
> + 50, 1500000);
> + baud_clock = readb(port->membase + CLK_SEL_REG);
> +
> + if ((baud_clock & CLK_SEL_MASK) == CLK_SEL_500MHZ) {
> + quot = 500000000 / (16 * baud);
> + writel(quot, (port->membase + CLK_DIVISOR_REG));
> + } else if ((baud_clock & CLK_SEL_MASK) == CLK_SEL_166MHZ) {
> + quot = (166667 * 1000) / (16 * baud);
> + writel(quot, (port->membase + CLK_DIVISOR_REG));
> + } else {
> + quot = ((1000000000) / (baud * UART_BIT_SAMPLE_CNT));
> + frac = (((1000000000 - (quot * baud *
> + UART_BIT_SAMPLE_CNT)) / UART_BIT_SAMPLE_CNT)
> + * 255) / baud;
> + writel(frac | (quot << 8),
> + (port->membase + CLK_DIVISOR_REG));
> + }
Please check ->[gs]et_divisor() out.
--
i.
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Wednesday, August 31, 2022 1:24 AM
> To: Kumaravel Thiagarajan - I21417 <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>; Jiri Slaby
> <[email protected]>; Ilpo Järvinen <[email protected]>; Uwe
> Kleine-König <[email protected]>; Johan Hovold
> <[email protected]>; Wander Lairson Costa <[email protected]>; Eric
> Tremblay <[email protected]>; Maciej W. Rozycki
> <[email protected]>; Geert Uytterhoeven <[email protected]>;
> Jeremy Kerr <[email protected]>; Phil Edworthy <[email protected]>;
> Lukas Wunner <[email protected]>; Linux Kernel Mailing List <linux-
> [email protected]>; open list:SERIAL DRIVERS <linux-
> [email protected]>; UNGLinuxDriver
> <[email protected]>
> Subject: Re: [PATCH v1 tty-next 1/2] 8250: microchip: pci1xxxx: Add driver for
> the quad-uart function in the multi-function endpoint of pci1xxxx device.
>
>
> On Tue, Aug 30, 2022 at 9:01 PM 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 the contribution!
> Brief looking into the code I can see that you may easily reduce it by ~20%.
> Think about it. You may take other examples, that are servicing PCI based
> devices (8250_exar, 8250_lpss, 8250_mid) on how to shrink the code base.
>
> ...
>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/string.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +#include <linux/delay.h>
> > +#include <linux/tty.h>
> > +#include <linux/serial_reg.h>
> > +#include <linux/serial_core.h>
> > +#include <linux/8250_pci.h>
> > +#include <linux/serial_8250.h>
> > +#include <linux/bitops.h>
> > +#include <linux/io.h>
>
> Why not sorted?
> Do you need all of them?
Ok. I will review and modify this if possible
>
> ...
>
> > + const unsigned int standard_baud_list[] = {50, 75, 110, 134, 150, 300,
> > + 600, 1200, 1800, 2000, 2400, 3600,
> > + 4800, 7200, 9600, 19200, 38400, 57600,
> > + 115200, 125000, 136400, 150000, 166700,
> > + 187500, 214300, 250000, 300000, 375000,
> > + 500000, 750000,
> > + 1000000, 1500000};
>
> Why?!
The standard baud rates are handled within serial8250_do_set_termios which is invoked from
within mchp_pci1xxxx_set_termios in first place. Hence if it matches with any of the standard baudrates,
it can return immediately.
>
> ...
>
> > + if (baud == 38400 && (port->flags & UPF_SPD_MASK) ==
> > + UPF_SPD_CUST) {
>
> No. We don't want to have this in the new drivers. There is BOTHER which
> might be used instead.
Ok. I will modify this.
>
> > + writel((port->custom_divisor & 0x3FFFFFFF),
> > + (port->membase + CLK_DIVISOR_REG));
>
> ...
>
> > + frac = (((1000000000 - (quot * baud *
> > + UART_BIT_SAMPLE_CNT)) / UART_BIT_SAMPLE_CNT)
> > + * 255) / baud;
>
> Funny indentation.
Ok. I will modify this.
>
> ...
>
> > +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);
>
> > + pci_save_state(dev);
>
> Why is this call here?
I think it should not be required. I will remove this, test with the device and fix this.
>
> > + if (rc)
> > + return rc;
> > +
> > + nr_ports = pci1xxxx_get_num_ports(dev);
> > +
> > + priv = devm_kzalloc(&dev->dev, struct_size(priv, line,
> > + nr_ports), GFP_KERNEL);
> > +
> > + priv->membase = pcim_iomap(dev, 0, 0);
> > + priv->dev = dev;
> > + priv->nr = nr_ports;
>
> > + if (!priv)
> > + return -ENOMEM;
>
> You are dereferencing a NULL pointer before checking, how did you test your
> code?
Ok. I will modify this.
>
> > + pci_set_master(dev);
> > +
> > + num_vectors = pci_alloc_irq_vectors(dev, 1, 4, PCI_IRQ_ALL_TYPES);
> > + if (num_vectors < 0)
> > + return rc;
>
> What does this mean?
It is a mistake. I will replace the num_vectors variable with rc.
>
> > + memset(&uart, 0, sizeof(uart));
> > + uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_TYPE |
> UPF_FIXED_PORT;
> > + uart.port.uartclk = 48000000;
> > + 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)
> > + mchp_pci1xxxx_irq_assign(priv, &uart, i);
> > + rc = mchp_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 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) },
>
> > + {0,}
>
> { } is enough
Ok. I will modify this.
>
> > +};
>
> ...
>
> > +
>
> Unneeded blank line
Ok. I will modify this.
>
> > +module_pci_driver(pci1xxxx_pci_driver);
>
> ...
>
> > + [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,
> > + },
>
> Why do you need this?
I think this is required because of the difference in the values of the FIFO trigger levels and FIFO depth.
I will review this.
>
> ...
>
> > +/* MCHP 16550A UART with 256 byte FIFOs */
> > +#define PORT_MCHP16550A 124
>
> ...and this?
> If you really need this, try to find a gap in the numbering, there are some.
Sure. I will review this.
>
> --
> With Best Regards,
> Andy Shevchenko
> -----Original Message-----
> From: Geert Uytterhoeven <[email protected]>
> Sent: Wednesday, August 31, 2022 1:28 AM
> To: Kumaravel Thiagarajan - I21417 <[email protected]>
> Cc: Greg KH <[email protected]>; Jiri Slaby <[email protected]>;
> Ilpo Jarvinen <[email protected]>; Andy Shevchenko
> <[email protected]>; Uwe Kleine-König <u.kleine-
> [email protected]>; Johan Hovold <[email protected]>;
> [email protected]; [email protected]; Maciej W. Rozycki
> <[email protected]>; Jeremy Kerr <[email protected]>; Phil Edworthy
> <[email protected]>; Lukas Wunner <[email protected]>; Linux
> Kernel Mailing List <[email protected]>; open list:SERIAL DRIVERS
> <[email protected]>; UNGLinuxDriver
> <[email protected]>
> Subject: Re: [PATCH v1 tty-next 1/2] 8250: microchip: pci1xxxx: Add driver for
> the quad-uart function in the multi-function endpoint of pci1xxxx device.
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> Hi Kumaravel,
>
> On Tue, Aug 30, 2022 at 8:01 PM 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.
> >
> > Signed-off-by: Kumaravel Thiagarajan
> > <[email protected]>
>
> Thanks for your patch!
>
> > --- /dev/null
> > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
>
> > +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);
>
> > --- a/drivers/tty/serial/8250/Kconfig
> > +++ b/drivers/tty/serial/8250/Kconfig
> > @@ -528,6 +528,15 @@ 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
>
> As this is a PCI driver, I guess it should depend on PCI (|| COMPILE_TEST)?
Ok. I will review this and modify as required.
>
> > + 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.
>
Thank You.
Regards,
Kumaravel
> -----Original Message-----
> From: Ilpo J?rvinen <[email protected]>
> Sent: Wednesday, August 31, 2022 3:13 PM
> To: Kumaravel Thiagarajan - I21417 <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>; Jiri Slaby
> <[email protected]>; [email protected]; u.kleine-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Lukas Wunner <[email protected]>; LKML <[email protected]>;
> linux-serial <[email protected]>; UNGLinuxDriver
> <[email protected]>
> Subject: Re: [PATCH v1 tty-next 1/2] 8250: microchip: pci1xxxx: Add driver for
> the quad-uart function in the multi-function endpoint of pci1xxxx device.
>
>
> On Tue, 30 Aug 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]>
> > ---
>
> > +static int mchp_pci1xxxx_rs485_config(struct uart_port *port,
> > + struct ktermios *termios,
> > + struct serial_rs485 *rs485) {
> > + u8 data = 0;
> > +
> > + memset(rs485->padding, 0, sizeof(rs485->padding));
> > + rs485->flags &= SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND;
> > +
> > + if (rs485->flags & SER_RS485_ENABLED) {
> > + 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;
> > + }
> > + }
> > +
> > + rs485->delay_rts_after_send = 0;
> > + rs485->delay_rts_before_send = 0;
> > + writeb(data, (port->membase + ADCL_CFG_REG));
> > + port->rs485 = *rs485;
>
> Most of this will be handled for by the core so don't duplicate it in the driver.
Ok. I will review and modify this if as required.
>
> These days, you also need to provide rs485_supported.
Ok. I will modify this.
>
> > + return 0;
> > +}
> > +
> > +static void mchp_pci1xxxx_set_termios(struct uart_port *port,
> > + struct ktermios *termios,
> > + struct ktermios *old) {
> > + const unsigned int standard_baud_list[] = {50, 75, 110, 134, 150, 300,
> > + 600, 1200, 1800, 2000, 2400, 3600,
> > + 4800, 7200, 9600, 19200, 38400, 57600,
> > + 115200, 125000, 136400, 150000, 166700,
> > + 187500, 214300, 250000, 300000, 375000,
> > + 500000, 750000, 1000000, 1500000};
> > + unsigned int baud = tty_termios_baud_rate(termios);
> > + unsigned int baud_clock;
> > + unsigned int quot;
> > + unsigned int frac;
> > + unsigned int i;
> > +
> > + baud = tty_termios_baud_rate(termios);
>
> Either this or the one at the declaration is redundant.
Yes. It is a mistake. I will modify.
>
> > + serial8250_do_set_termios(port, termios, NULL);
> > +
> > + if (baud == 38400 && (port->flags & UPF_SPD_MASK) ==
> UPF_SPD_CUST) {
> > + writel((port->custom_divisor & 0x3FFFFFFF),
> > + (port->membase + CLK_DIVISOR_REG));
> > + } else {
> > + for (i = 0; i < ARRAY_SIZE(standard_baud_list); i++) {
> > + if (baud == standard_baud_list[i])
> > + return;
>
> Did you mean to break here instead?
No. The standard baud rates are handled within serial8250_do_set_termios which is invoked from
within mchp_pci1xxxx_set_termios in first place. Hence, if it matches with any of the standard baudrates,
it can return immediately.
>
> > + }
> > + tty_termios_encode_baud_rate(termios, baud, baud);
> > +
> > + baud = uart_get_baud_rate(port, termios, old,
> > + 50, 1500000);
> > + baud_clock = readb(port->membase + CLK_SEL_REG);
> > +
> > + if ((baud_clock & CLK_SEL_MASK) == CLK_SEL_500MHZ) {
> > + quot = 500000000 / (16 * baud);
> > + writel(quot, (port->membase + CLK_DIVISOR_REG));
> > + } else if ((baud_clock & CLK_SEL_MASK) == CLK_SEL_166MHZ) {
> > + quot = (166667 * 1000) / (16 * baud);
> > + writel(quot, (port->membase + CLK_DIVISOR_REG));
> > + } else {
> > + quot = ((1000000000) / (baud * UART_BIT_SAMPLE_CNT));
> > + frac = (((1000000000 - (quot * baud *
> > + UART_BIT_SAMPLE_CNT)) / UART_BIT_SAMPLE_CNT)
> > + * 255) / baud;
> > + writel(frac | (quot << 8),
> > + (port->membase + CLK_DIVISOR_REG));
> > + }
>
> Please check ->[gs]et_divisor() out.
Ok. I will review and get back to you.
Thank You.
Regards,
Kumaravel