[PATCH v1 tty-next 0/2] 8250: microchip: pci1xxxx: Add driver for the pci1xxxx's quad-uart function.

[PATCH v1 tty-next 0/2] 8250: microchip: pci1xxxx: Add driver for the pci1xxxx's quad-uart function.

[Kumaravel Thiagarajan]

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

[PATCH v1 tty-next 1/2] 8250: microchip: pci1xxxx: Add driver for the quad-uart function in the multi-function endpoint of pci1xxxx device.

[Kumaravel Thiagarajan]

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 <kumaravel.thiagarajan@microchip.com>
---
 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 <kumaravel.thiagarajan@microchip.com>
+L:	linux-serial@vger.kernel.org
+S:	Maintained
+F:	drivers/tty/serial/8250/8250_pci1xxxx.c
+
 8390 NETWORK DRIVERS [WD80x3/SMC-ELITE, SMC-ULTRA, NE2000, 3C503, etc.]
 L:	netdev@vger.kernel.org
 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 <kumaravel.thiagarajan@microchip.com>");
+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

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.

[Andy Shevchenko]

On Tue, Aug 30, 2022 at 9:01 PM Kumaravel Thiagarajan
<kumaravel.thiagarajan@microchip.com> 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

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.

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Wednesday, August 31, 2022 1:24 AM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>; Uwe
> Kleine-König <u.kleine-koenig@pengutronix.de>; Johan Hovold
> <johan@kernel.org>; Wander Lairson Costa <wander@redhat.com>; Eric
> Tremblay <etremblay@distech-controls.com>; Maciej W. Rozycki
> <macro@orcam.me.uk>; Geert Uytterhoeven <geert+renesas@glider.be>;
> Jeremy Kerr <jk@ozlabs.org>; Phil Edworthy <phil.edworthy@renesas.com>;
> Lukas Wunner <lukas@wunner.de>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; open list:SERIAL DRIVERS <linux-
> serial@vger.kernel.org>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>
> 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
> <kumaravel.thiagarajan@microchip.com> 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

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.

[Ilpo Järvinen]

On Thu, 1 Sep 2022, Kumaravel.Thiagarajan@microchip.com wrote:

> > > +       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.

Care to explain why the baudrates in your table don't match those in 
tty_baudrate.c? ...It makes no sense to me that you call these "standard 
baud rates".


-- 
 i.

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.

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: Thursday, September 1, 2022 7:12 PM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Cc: andy.shevchenko@gmail.com; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Jiri Slaby <jirislaby@kernel.org>; u.kleine-
> koenig@pengutronix.de; johan@kernel.org; wander@redhat.com;
> etremblay@distech-controls.com; macro@orcam.me.uk;
> geert+renesas@glider.be; jk@ozlabs.org; phil.edworthy@renesas.com;
> Lukas Wunner <lukas@wunner.de>; LKML <linux-kernel@vger.kernel.org>;
> linux-serial <linux-serial@vger.kernel.org>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>
> 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 Thu, 1 Sep 2022, Kumaravel.Thiagarajan@microchip.com wrote:
> 
> > > > +       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.
> 
> Care to explain why the baudrates in your table don't match those in
> tty_baudrate.c? ...It makes no sense to me that you call these "standard
> baud rates".
The baudrates in my table are from our legacy UART IP and these baudrates can be
generated by the hardware by updating UART_DLL & UART_DLM alone as done by the
serial8250_do_set_termios.
I noticed that some of the baud rates in tty_baudrate.c arenot listed in this table
but will still be handled by the mchp_pci1xxxx_set_termios.
I can rename standard_baud_list to simply baud_list. Please let me know.

Thank You.

Regards,
Kumaravel

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.

[Andy Shevchenko]

On Fri, Sep 2, 2022 at 2:57 PM <Kumaravel.Thiagarajan@microchip.com> wrote:
> > -----Original Message-----
> > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Sent: Thursday, September 1, 2022 7:12 PM
> > On Thu, 1 Sep 2022, Kumaravel.Thiagarajan@microchip.com wrote:

...

> > > > > +       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.
> >
> > Care to explain why the baudrates in your table don't match those in
> > tty_baudrate.c? ...It makes no sense to me that you call these "standard
> > baud rates".
> The baudrates in my table are from our legacy UART IP and these baudrates can be
> generated by the hardware by updating UART_DLL & UART_DLM alone as done by the
> serial8250_do_set_termios.
> I noticed that some of the baud rates in tty_baudrate.c arenot listed in this table
> but will still be handled by the mchp_pci1xxxx_set_termios.
> I can rename standard_baud_list to simply baud_list. Please let me know.

No, the point is avoid repeating what standard APIs already do. Just
make sure you call it properly and provide _get/_set_divisor()
callbacks. Note, your driver can cope with BOTHER and there all
non-standard baud rates go.

-- 
With Best Regards,
Andy Shevchenko

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.

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Friday, September 2, 2022 8:33 PM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Jiri Slaby <jirislaby@kernel.org>; Uwe
> Kleine-König <u.kleine-koenig@pengutronix.de>; Johan Hovold
> <johan@kernel.org>; Wander Lairson Costa <wander@redhat.com>; Eric
> Tremblay <etremblay@distech-controls.com>; Maciej W. Rozycki
> <macro@orcam.me.uk>; Geert Uytterhoeven <geert+renesas@glider.be>;
> Jeremy Kerr <jk@ozlabs.org>; Phil Edworthy <phil.edworthy@renesas.com>;
> Lukas Wunner <lukas@wunner.de>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; open list:SERIAL DRIVERS <linux-
> serial@vger.kernel.org>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>
> 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 Fri, Sep 2, 2022 at 2:57 PM <Kumaravel.Thiagarajan@microchip.com>
> wrote:
> > > -----Original Message-----
> > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > Sent: Thursday, September 1, 2022 7:12 PM On Thu, 1 Sep 2022,
> > > Kumaravel.Thiagarajan@microchip.com wrote:
> 
> ...
> 
> > > > > > +       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.
> > >
> > > Care to explain why the baudrates in your table don't match those in
> > > tty_baudrate.c? ...It makes no sense to me that you call these
> > > "standard baud rates".
> > The baudrates in my table are from our legacy UART IP and these
> > baudrates can be generated by the hardware by updating UART_DLL &
> > UART_DLM alone as done by the serial8250_do_set_termios.
> > I noticed that some of the baud rates in tty_baudrate.c arenot listed
> > in this table but will still be handled by the mchp_pci1xxxx_set_termios.
> > I can rename standard_baud_list to simply baud_list. Please let me know.
> 
> No, the point is avoid repeating what standard APIs already do. Just make
> sure you call it properly and provide _get/_set_divisor() callbacks. Note, your
> driver can cope with BOTHER and there all non-standard baud rates go.
I will review my driver again and get back to you if required.

Thank You.

Regards,
Kumaravel

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.

[Geert Uytterhoeven]

Hi Kumaravel,

On Tue, Aug 30, 2022 at 8:01 PM Kumaravel Thiagarajan
<kumaravel.thiagarajan@microchip.com> 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 <kumaravel.thiagarajan@microchip.com>

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 -- geert@linux-m68k.org

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

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.

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Wednesday, August 31, 2022 1:28 AM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>; Jiri Slaby <jirislaby@kernel.org>;
> Ilpo Jarvinen <ilpo.jarvinen@linux.intel.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; Uwe Kleine-König <u.kleine-
> koenig@pengutronix.de>; Johan Hovold <johan@kernel.org>;
> wander@redhat.com; etremblay@distech-controls.com; Maciej W. Rozycki
> <macro@orcam.me.uk>; Jeremy Kerr <jk@ozlabs.org>; Phil Edworthy
> <phil.edworthy@renesas.com>; Lukas Wunner <lukas@wunner.de>; Linux
> Kernel Mailing List <linux-kernel@vger.kernel.org>; open list:SERIAL DRIVERS
> <linux-serial@vger.kernel.org>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>
> 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
> <kumaravel.thiagarajan@microchip.com> 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
> > <kumaravel.thiagarajan@microchip.com>
> 
> 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

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.

[kernel test robot]

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/202208310659.5tM9wAHE-lkp@intel.com/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 <lkp@intel.com>

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

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.

[kernel test robot]

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/202208310603.2xUZ8ee9-lkp@intel.com/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 <lkp@intel.com>

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

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.

[Ilpo Järvinen]

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 <kumaravel.thiagarajan@microchip.com>
> ---

> +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.

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.

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: Wednesday, August 31, 2022 3:13 PM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; andy.shevchenko@gmail.com; u.kleine-
> koenig@pengutronix.de; johan@kernel.org; wander@redhat.com;
> etremblay@distech-controls.com; macro@orcam.me.uk;
> geert+renesas@glider.be; jk@ozlabs.org; phil.edworthy@renesas.com;
> Lukas Wunner <lukas@wunner.de>; LKML <linux-kernel@vger.kernel.org>;
> linux-serial <linux-serial@vger.kernel.org>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>
> 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
> > <kumaravel.thiagarajan@microchip.com>
> > ---
> 
> > +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

[PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver.

[Kumaravel Thiagarajan]

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 upon resume.

Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
---
 drivers/tty/serial/8250/8250_pci1xxxx.c | 122 ++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
index 56852ae0585e..38c2a6a9e5d5 100644
--- a/drivers/tty/serial/8250/8250_pci1xxxx.c
+++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
@@ -70,6 +70,7 @@
 
 #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
@@ -78,6 +79,9 @@
 #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 {
@@ -439,6 +443,121 @@ static void pci1xxxx_serial_remove(struct pci_dev *dev)
 		serial8250_unregister_port(priv->line[i]);
 }
 
+#ifdef CONFIG_PM_SLEEP
+
+static char 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;
+	char ret = 0;
+
+	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 = 0x01;
+		else
+			ret = 0x00;
+	}
+
+	writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
+
+	return ret;
+}
+
+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;
+	char wakeup = 0;
+	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;
+}
+
+#endif
+
+static 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) },
@@ -453,6 +572,9 @@ static struct pci_driver pci1xxxx_pci_driver = {
 	.name		= "pci1xxxx serial",
 	.probe		= pci1xxxx_serial_probe,
 	.remove	= pci1xxxx_serial_remove,
+	.driver	= {
+		.pm     = &pci1xxxx_pm_ops,
+	},
 	.id_table	= pci1xxxx_pci_tbl,
 };
 
-- 
2.25.1

Re: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver.

[Andy Shevchenko]

On Tue, Aug 30, 2022 at 9:01 PM Kumaravel Thiagarajan
<kumaravel.thiagarajan@microchip.com> 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 upon resume.

on resume

...

First of all, we have pm_ptr() and pm_sleep_ptr() with corresponding
other macros in pm.h. Use them.
Second, if you need to duplicate a lot of code from 8250_pci, split
that code to 8250_pcilib.c and use it in the driver.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver.

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Wednesday, August 31, 2022 1:26 AM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>; Uwe
> Kleine-König <u.kleine-koenig@pengutronix.de>; Johan Hovold
> <johan@kernel.org>; Wander Lairson Costa <wander@redhat.com>; Eric
> Tremblay <etremblay@distech-controls.com>; Maciej W. Rozycki
> <macro@orcam.me.uk>; Geert Uytterhoeven <geert+renesas@glider.be>;
> Jeremy Kerr <jk@ozlabs.org>; Phil Edworthy <phil.edworthy@renesas.com>;
> Lukas Wunner <lukas@wunner.de>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; open list:SERIAL DRIVERS <linux-
> serial@vger.kernel.org>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>
> Subject: Re: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power
> management functions to pci1xxxx's quad-uart driver.
> 
> 
> On Tue, Aug 30, 2022 at 9:01 PM Kumaravel Thiagarajan
> <kumaravel.thiagarajan@microchip.com> 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 upon resume.
> 
> on resume
Ok. I will modify this.

> 
> ...
> 
> First of all, we have pm_ptr() and pm_sleep_ptr() with corresponding other
> macros in pm.h. Use them.
> Second, if you need to duplicate a lot of code from 8250_pci, split that code
> to 8250_pcilib.c and use it in the driver.
Ok. I will review and get back to you.

Thank You.

Regards,
Kumaravel

RE: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver.

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Kumaravel Thiagarajan - I21417
> <Kumaravel.Thiagarajan@microchip.com>
> Sent: Thursday, September 1, 2022 7:19 PM
> To: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> > -----Original Message-----
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Wednesday, August 31, 2022 1:26 AM
> > To: Kumaravel Thiagarajan - I21417
> > <Kumaravel.Thiagarajan@microchip.com>
> >
> >
> > On Tue, Aug 30, 2022 at 9:01 PM Kumaravel Thiagarajan
> > <kumaravel.thiagarajan@microchip.com> 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 upon resume.
> >
> > on resume
> Ok. I will modify this.
> 
> >
> > ...
> >
> > First of all, we have pm_ptr() and pm_sleep_ptr() with corresponding
> > other macros in pm.h. Use them.
> > Second, if you need to duplicate a lot of code from 8250_pci, split
> > that code to 8250_pcilib.c and use it in the driver.
> Ok. I will review and get back to you.
Andy, I was able to start on the v2 for the patch only a few days ago and about to complete it now.
I have absorbed most of the changes suggested whereas for the above change I felt it may not be
required to split 8250_pci.c at least now as there is only one function setup_port which I have
duplicated in 8250_pci1xxxx.c. Please let me know your thoughts.
I will submit the v2 patch for review soon.

Thank You.

Regards,
Kumar

Re: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver.

[kernel test robot]

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: mips-allyesconfig (https://download.01.org/0day-ci/archive/20220831/202208310603.ea8ISalW-lkp@intel.com/config)
compiler: mips-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/a994946078a1bca8361d4f3245ad306515291b6e
        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 a994946078a1bca8361d4f3245ad306515291b6e
        # 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=mips SHELL=/bin/bash drivers/tty/serial/8250/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

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:293: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]
     293 |         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:305:6: warning: no previous prototype for 'mchp_pci1xxxx_irq_assign' [-Wmissing-prototypes]
     305 | void mchp_pci1xxxx_irq_assign(struct pci1xxxx_8250 *priv,
         |      ^~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/tty/serial/8250/8250_pci1xxxx.c:475:6: warning: no previous prototype for 'pci1xxxx_port_resume' [-Wmissing-prototypes]
     475 | void pci1xxxx_port_resume(int line)
         |      ^~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/pci1xxxx_port_resume +475 drivers/tty/serial/8250/8250_pci1xxxx.c

   474	
 > 475	void pci1xxxx_port_resume(int line)
   476	{
   477		struct uart_8250_port *up = serial8250_get_port(line);
   478		struct uart_port *port = &up->port;
   479		unsigned long flags;
   480	
   481		writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
   482	
   483		if (port->suspended == 0) {
   484			spin_lock_irqsave(&port->lock, flags);
   485			port->mctrl |= TIOCM_OUT2;
   486			port->ops->set_mctrl(port, port->mctrl);
   487			spin_unlock_irqrestore(&port->lock, flags);
   488		}
   489	}
   490	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver.

[Ilpo Järvinen]

On Tue, 30 Aug 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 upon resume.
> 
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> ---
>  drivers/tty/serial/8250/8250_pci1xxxx.c | 122 ++++++++++++++++++++++++
>  1 file changed, 122 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
> index 56852ae0585e..38c2a6a9e5d5 100644
> --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> @@ -70,6 +70,7 @@
>  
>  #define UART_PCI_CTRL_REG		0x80
>  #define UART_PCI_CTRL_SET_MULTIPLE_MSI	BIT(4)
> +#define UART_PCI_CTRL_D3_CLK_ENABLE	BIT(0)

Reorder.

> +static char pci1xxxx_port_suspend(int line)

Why char???

> +{
> +	struct uart_8250_port *up = serial8250_get_port(line);
> +	struct uart_port *port = &up->port;
> +	unsigned long flags;
> +	u8 wakeup_mask;
> +	char ret = 0;
> +
> +	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 = 0x01;
> +		else
> +			ret = 0x00;

Is it a bool or should you name these it with a #define?

> +	}
> +
> +	writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> +
> +	return ret;
> +}
> +
> +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;
> +	char wakeup = 0;
> +	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)

It looks you'd want bool instead of char here.


-- 
 i.

RE: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver.

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: Wednesday, August 31, 2022 3:24 PM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; andy.shevchenko@gmail.com; u.kleine-
> koenig@pengutronix.de; johan@kernel.org; wander@redhat.com;
> etremblay@distech-controls.com; macro@orcam.me.uk;
> geert+renesas@glider.be; jk@ozlabs.org; phil.edworthy@renesas.com;
> Lukas Wunner <lukas@wunner.de>; LKML <linux-kernel@vger.kernel.org>;
> linux-serial <linux-serial@vger.kernel.org>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>
> Subject: Re: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power
> management functions to pci1xxxx's quad-uart driver.
> 
> 
> On Tue, 30 Aug 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 upon resume.
> >
> > Signed-off-by: Kumaravel Thiagarajan
> > <kumaravel.thiagarajan@microchip.com>
> > ---
> >  drivers/tty/serial/8250/8250_pci1xxxx.c | 122
> > ++++++++++++++++++++++++
> >  1 file changed, 122 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > index 56852ae0585e..38c2a6a9e5d5 100644
> > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > @@ -70,6 +70,7 @@
> >
> >  #define UART_PCI_CTRL_REG            0x80
> >  #define UART_PCI_CTRL_SET_MULTIPLE_MSI       BIT(4)
> > +#define UART_PCI_CTRL_D3_CLK_ENABLE  BIT(0)
> 
> Reorder.
I have ordered this like - Register offset followed by individual bits from MSB to LSB.
Should I reorder this based on some other criteria?

> 
> > +static char pci1xxxx_port_suspend(int line)
> 
> Why char???
I will modify this to bool.

> 
> > +{
> > +     struct uart_8250_port *up = serial8250_get_port(line);
> > +     struct uart_port *port = &up->port;
> > +     unsigned long flags;
> > +     u8 wakeup_mask;
> > +     char ret = 0;
> > +
> > +     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 = 0x01;
> > +             else
> > +                     ret = 0x00;
> 
> Is it a bool or should you name these it with a #define?
bool is the correct data type. I will fix this.

> 
> > +     }
> > +
> > +     writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> > +
> > +     return ret;
> > +}
> > +
> > +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;
> > +     char wakeup = 0;
> > +     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)
> 
> It looks you'd want bool instead of char here.
Yes. I will modify this to bool.

Thank You.

Regards,
Kumaravel