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

[PATCH v2 tty-next 0/3] 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.

The changes from v1 are mentioned in each patch in the patchset.

Thanks to Andy Shevchenko, Ilpo Jarvinen, Geert Uytterhoeven for their
review comments for v1.

Kumaravel Thiagarajan (3):
  8250: microchip: pci1xxxx: Add driver for quad-uart support.
  8250: microchip: pci1xxxx: Add rs485 support to quad-uart driver.
  8250: microchip: pci1xxxx: Add power management functions to quad-uart
    driver.

 MAINTAINERS                             |   6 +
 drivers/tty/serial/8250/8250_pci1xxxx.c | 563 ++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_port.c     |   8 +
 drivers/tty/serial/8250/Kconfig         |  10 +
 drivers/tty/serial/8250/Makefile        |   1 +
 include/uapi/linux/serial_core.h        |   3 +
 6 files changed, 591 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_pci1xxxx.c

-- 
2.25.1

[PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[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>
---
Changes in v2:
- Use only the 62.5 MHz for baud clock.
- Define custom implementation for get_divisor and set_divisor.
- Use BOTHER instead of UPF_SPD_CUST for non standard baud rates (untested).
- Correct indentation in clock divisor computation.
- Remove unnecessary call to pci_save_state in probe function.
- Fix null pointer dereference in probe function.
- Move pci1xxxx_rs485_config to a separate patch.
- Depends on SERIAL_8250_PCI & default to SERIAL_8250.
- Change PORT_MCHP16550A to 100 from 124.
--- 
 MAINTAINERS                             |   6 +
 drivers/tty/serial/8250/8250_pci1xxxx.c | 394 ++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_port.c     |   8 +
 drivers/tty/serial/8250/Kconfig         |  10 +
 drivers/tty/serial/8250/Makefile        |   1 +
 include/uapi/linux/serial_core.h        |   3 +
 6 files changed, 422 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_pci1xxxx.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d30f26e07cd3..3390693d57ae 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -218,6 +218,12 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
 F:	drivers/tty/serial/8250*
 F:	include/linux/serial_8250.h
 
+MICROCHIP PCIe UART DRIVER
+M:	Kumaravel Thiagarajan <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..41a4b94f52b4
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
@@ -0,0 +1,394 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *  Probe module for 8250/16550-type MCHP PCI serial ports.
+ *
+ *  Based on drivers/tty/serial/8250/8250_pci.c,
+ *
+ *  Copyright (C) 2022 Microchip Technology Inc., All Rights Reserved.
+ */
+
+#include <linux/serial_core.h>
+#include <linux/8250_pci.h>
+#include <asm/byteorder.h>
+#include <linux/bitops.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/pci.h>
+#include <linux/tty.h>
+#include <linux/io.h>
+#include "8250.h"
+
+#define PCI_VENDOR_ID_MCHP_PCI1XXXX	0x1055
+
+#define PCI_DEVICE_ID_MCHP_PCI12000	0xA002
+#define PCI_DEVICE_ID_MCHP_PCI11010	0xA012
+#define PCI_DEVICE_ID_MCHP_PCI11101	0xA022
+#define PCI_DEVICE_ID_MCHP_PCI11400	0xA032
+#define PCI_DEVICE_ID_MCHP_PCI11414	0xA042
+
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p	0x0001
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p012	0x0002
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013	0x0003
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023	0x0004
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123	0x0005
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p01	0x0006
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02	0x0007
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03	0x0008
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12	0x0009
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13	0x000A
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23	0x000B
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p0	0x000C
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1	0x000D
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2	0x000E
+#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3	0x000F
+
+#define PCI_SUBDEVICE_ID_MCHP_PCI12000	0xA002
+#define PCI_SUBDEVICE_ID_MCHP_PCI11010	0xA012
+#define PCI_SUBDEVICE_ID_MCHP_PCI11101	0xA022
+#define PCI_SUBDEVICE_ID_MCHP_PCI11400	0xA032
+#define PCI_SUBDEVICE_ID_MCHP_PCI11414	0xA042
+
+#define UART_ACTV_REG			0x11
+#define UART_ACTV_SET_ACTIVE		BIT(0)
+
+#define ADCL_CFG_REG			0x40
+#define ADCL_CFG_POL_SEL		BIT(2)
+#define ADCL_CFG_PIN_SEL		BIT(1)
+#define ADCL_CFG_EN			BIT(0)
+
+#define CLK_SEL_REG			0x50
+#define CLK_SEL_MASK			GENMASK(1, 0)
+#define CLK_SEL_166MHZ			0x01
+#define CLK_SEL_500MHZ			0x02
+
+#define CLK_DIVISOR_REG			0x54
+
+#define UART_PCI_CTRL_REG		0x80
+#define UART_PCI_CTRL_SET_MULTIPLE_MSI	BIT(4)
+#define UART_PCI_CTRL_D3_CLK_ENABLE	BIT(0)
+
+#define UART_WAKE_REG			0x8C
+#define UART_WAKE_MASK_REG		0x90
+#define UART_WAKE_N_PIN			BIT(2)
+#define UART_WAKE_NCTS			BIT(1)
+#define UART_WAKE_INT			BIT(0)
+#define UART_WAKE_SRCS			(UART_WAKE_N_PIN | UART_WAKE_NCTS | UART_WAKE_INT)
+
+#define UART_RESET_REG			0x94
+#define UART_RESET_D3_RESET_DISABLE	BIT(16)
+
+#define UART_BIT_SAMPLE_CNT 16
+
+struct pci1xxxx_8250 {
+	struct pci_dev		*dev;
+	unsigned int		nr;
+	void __iomem		*membase;
+	int			line[];
+};
+
+static int pci1xxxx_setup_port(struct pci1xxxx_8250 *priv, struct uart_8250_port *port,
+			       int offset)
+{
+	struct pci_dev *dev = priv->dev;
+
+	if (pci_resource_flags(dev, 0) & IORESOURCE_MEM) {
+		if (!pcim_iomap(dev, 0, 0) && !pcim_iomap_table(dev))
+			return -ENOMEM;
+
+		port->port.iotype = UPIO_MEM;
+		port->port.iobase = 0;
+		port->port.mapbase = pci_resource_start(dev, 0) + offset;
+		port->port.membase = pcim_iomap_table(dev)[0] + offset;
+		port->port.regshift = 0;
+	} else {
+		port->port.iotype = UPIO_PORT;
+		port->port.iobase = pci_resource_start(dev, 0) + offset;
+		port->port.mapbase = 0;
+		port->port.membase = NULL;
+		port->port.regshift = 0;
+	}
+
+	return 0;
+}
+
+static unsigned int pci1xxxx_get_divisor(struct uart_port *port,
+					 unsigned int baud,
+					 unsigned int *frac)
+{
+	unsigned int quot;
+
+	/*
+	 * Calculate baud rate sampling period in nano seconds.
+	 * Fractional part x denotes x/255 parts of a nano second.
+	 */
+
+	quot = ((1000000000) / (baud * UART_BIT_SAMPLE_CNT));
+	*frac = (((1000000000 - (quot * baud * UART_BIT_SAMPLE_CNT)) /
+		  UART_BIT_SAMPLE_CNT) * 255) / baud;
+
+	return quot;
+}
+
+static void pci1xxxx_set_divisor(struct uart_port *port, unsigned int baud,
+				 unsigned int quot, unsigned int frac)
+{
+	writel((quot << 8) | frac, (port->membase + CLK_DIVISOR_REG));
+}
+
+static int pci1xxxx_get_num_ports(struct pci_dev *dev)
+{
+	int nr_ports = 1;
+
+	switch (dev->subsystem_device) {
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p0:
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1:
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2:
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3:
+		nr_ports = 1;
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p01:
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02:
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03:
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12:
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13:
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23:
+		nr_ports = 2;
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p012:
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123:
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013:
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023:
+		nr_ports = 3;
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p:
+	case PCI_SUBDEVICE_ID_MCHP_PCI11414:
+		nr_ports = 4;
+		break;
+	}
+
+	return nr_ports;
+}
+
+static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
+			  struct uart_8250_port *port, int idx)
+{
+	int first_offset = 0;
+	int offset;
+	int ret;
+
+	switch (priv->dev->subsystem_device) {
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1:
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12:
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123:
+		first_offset = 256;
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2:
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23:
+		first_offset = 512;
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3:
+		first_offset = 768;
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02:
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023:
+		if (idx > 0)
+			idx++;
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03:
+		if (idx > 0)
+			idx += 2;
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13:
+		first_offset = 256;
+		if (idx > 0)
+			idx++;
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013:
+		if (idx > 1)
+			idx++;
+		break;
+	}
+
+	offset = first_offset + (idx * 256);
+	port->port.flags |= UPF_FIXED_TYPE | UPF_SKIP_TEST;
+	port->port.type = PORT_MCHP16550A;
+	port->port.set_termios = serial8250_do_set_termios;
+	port->port.get_divisor = pci1xxxx_get_divisor;
+	port->port.set_divisor = pci1xxxx_set_divisor;
+	ret = pci1xxxx_setup_port(priv, port, offset);
+	if (ret < 0)
+		return ret;
+
+	writeb(UART_ACTV_SET_ACTIVE, port->port.membase + UART_ACTV_REG);
+	writeb(UART_WAKE_SRCS, port->port.membase + UART_WAKE_REG);
+	writeb(UART_WAKE_N_PIN, port->port.membase + UART_WAKE_MASK_REG);
+
+	return 0;
+}
+
+static void pci1xxxx_irq_assign(struct pci1xxxx_8250 *priv,
+				struct uart_8250_port *uart, int idx)
+{
+	switch (priv->dev->subsystem_device) {
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p0:
+	case PCI_SUBDEVICE_ID_MCHP_PCI12000:
+	case PCI_SUBDEVICE_ID_MCHP_PCI11010:
+	case PCI_SUBDEVICE_ID_MCHP_PCI11101:
+	case PCI_SUBDEVICE_ID_MCHP_PCI11400:
+		uart->port.irq = pci_irq_vector(priv->dev, 0);
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1:
+		uart->port.irq = pci_irq_vector(priv->dev, 1);
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2:
+		uart->port.irq = pci_irq_vector(priv->dev, 2);
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3:
+		uart->port.irq = pci_irq_vector(priv->dev, 3);
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p01:
+		uart->port.irq = pci_irq_vector(priv->dev, idx);
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02:
+		if (idx > 0)
+			idx++;
+		uart->port.irq = pci_irq_vector(priv->dev, idx);
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03:
+		if (idx > 0)
+			idx += 2;
+		uart->port.irq = pci_irq_vector(priv->dev, idx);
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12:
+		uart->port.irq = pci_irq_vector(priv->dev, idx + 1);
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13:
+		if (idx > 0)
+			idx += 1;
+		uart->port.irq = pci_irq_vector(priv->dev, idx + 1);
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23:
+		uart->port.irq = pci_irq_vector(priv->dev, idx + 2);
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p012:
+		uart->port.irq = pci_irq_vector(priv->dev, idx);
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013:
+		if (idx > 1)
+			idx++;
+		uart->port.irq = pci_irq_vector(priv->dev, idx);
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023:
+		if (idx > 0)
+			idx++;
+		uart->port.irq = pci_irq_vector(priv->dev, idx);
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123:
+		uart->port.irq = pci_irq_vector(priv->dev, idx + 1);
+		break;
+	case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p:
+	case PCI_SUBDEVICE_ID_MCHP_PCI11414:
+		uart->port.irq = pci_irq_vector(priv->dev, idx);
+		break;
+	}
+}
+
+static int pci1xxxx_serial_probe(struct pci_dev *dev,
+				 const struct pci_device_id *ent)
+{
+	struct pci1xxxx_8250 *priv;
+	struct uart_8250_port uart;
+	unsigned int nr_ports, i;
+	int num_vectors = 0;
+	int rc;
+
+	rc = pcim_enable_device(dev);
+	if (rc)
+		return rc;
+
+	nr_ports = pci1xxxx_get_num_ports(dev);
+
+	priv = devm_kzalloc(&dev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->membase = pcim_iomap(dev, 0, 0);
+	priv->dev = dev;
+	priv->nr =  nr_ports;
+
+	pci_set_master(dev);
+
+	num_vectors  = pci_alloc_irq_vectors(dev, 1, 4, PCI_IRQ_ALL_TYPES);
+	if (num_vectors < 0)
+		return num_vectors;
+
+	memset(&uart, 0, sizeof(uart));
+	uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_TYPE | UPF_FIXED_PORT;
+	uart.port.uartclk = 62500000;
+	uart.port.dev = &dev->dev;
+
+	if (num_vectors == 4)
+		writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI, (priv->membase + UART_PCI_CTRL_REG));
+	else
+		uart.port.irq = pci_irq_vector(dev, 0);
+
+	for (i = 0; i < nr_ports; i++) {
+		if (num_vectors == 4)
+			pci1xxxx_irq_assign(priv, &uart, i);
+		priv->line[i] = -ENOSPC;
+		rc = pci1xxxx_setup(priv, &uart, i);
+		if (rc) {
+			dev_err(&dev->dev, "Failed to setup port %u\n", i);
+			break;
+		}
+		priv->line[i] = serial8250_register_8250_port(&uart);
+
+		if (priv->line[i] < 0) {
+			dev_err(&dev->dev,
+				"Couldn't register serial port %lx, irq %d, type %d, error %d\n",
+				uart.port.iobase, uart.port.irq,
+				uart.port.iotype, priv->line[i]);
+			break;
+		}
+	}
+
+	pci_set_drvdata(dev, priv);
+
+	return 0;
+}
+
+static void pci1xxxx_serial_remove(struct pci_dev *dev)
+{
+	struct pci1xxxx_8250 *priv = pci_get_drvdata(dev);
+	int i;
+
+	for (i = 0; i < priv->nr; i++) {
+		if (priv->line[i] >= 0)
+			serial8250_unregister_port(priv->line[i]);
+	}
+}
+
+static const struct pci_device_id pci1xxxx_pci_tbl[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11010) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11101) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11400) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11414) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI12000) },
+	{}
+};
+MODULE_DEVICE_TABLE(pci, pci1xxxx_pci_tbl);
+
+static struct pci_driver pci1xxxx_pci_driver = {
+	.name		= "pci1xxxx serial",
+	.probe		= pci1xxxx_serial_probe,
+	.remove		= pci1xxxx_serial_remove,
+	.id_table	= pci1xxxx_pci_tbl,
+};
+
+module_pci_driver(pci1xxxx_pci_driver);
+
+MODULE_DESCRIPTION("Microchip Technology Inc. PCIe to UART module");
+MODULE_AUTHOR("Kumaravel Thiagarajan <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 1d2a43214b48..ec2fe5fd7b02 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -313,6 +313,14 @@ static const struct serial8250_config uart_config[] = {
 		.rxtrig_bytes	= {1, 4, 8, 14},
 		.flags		= UART_CAP_FIFO,
 	},
+	[PORT_MCHP16550A] = {
+		.name           = "MCHP16550A",
+		.fifo_size      = 256,
+		.tx_loadsz      = 256,
+		.fcr            = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01,
+		.rxtrig_bytes   = {2, 66, 130, 194},
+		.flags          = UART_CAP_FIFO,
+	},
 };
 
 /* Uart divisor latch read */
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index d0b49e15fbf5..a886727ff7bf 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -528,6 +528,16 @@ config SERIAL_8250_TEGRA
 	  Select this option if you have machine with an NVIDIA Tegra SoC and
 	  wish to enable 8250 serial driver for the Tegra serial interfaces.
 
+config SERIAL_8250_PCI1XXXX
+	tristate "Microchip 8250 based serial port"
+	depends on SERIAL_8250_PCI
+	default SERIAL_8250
+	help
+	 Select this option if you have a setup with Microchip PCIe
+	 Switch with serial port enabled and wish to enable 8250
+	 serial driver for the serial interface. This driver support
+	 will ensure to support baud rates upto 1.5Mpbs.
+
 config SERIAL_8250_BCM7271
 	tristate "Broadcom 8250 based serial port"
 	depends on SERIAL_8250 && (ARCH_BRCMSTB || COMPILE_TEST)
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index bee908f99ea0..e900ff11e321 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_SERIAL_8250_LPSS)		+= 8250_lpss.o
 obj-$(CONFIG_SERIAL_8250_MID)		+= 8250_mid.o
 obj-$(CONFIG_SERIAL_8250_PERICOM)	+= 8250_pericom.o
 obj-$(CONFIG_SERIAL_8250_PXA)		+= 8250_pxa.o
+obj-$(CONFIG_SERIAL_8250_PCI1XXXX)	+= 8250_pci1xxxx.o
 obj-$(CONFIG_SERIAL_8250_TEGRA)		+= 8250_tegra.o
 obj-$(CONFIG_SERIAL_8250_BCM7271)	+= 8250_bcm7271.o
 obj-$(CONFIG_SERIAL_OF_PLATFORM)	+= 8250_of.o
diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index 3ba34d8378bd..281fa286555c 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -207,6 +207,9 @@
 /* Atheros AR933X SoC */
 #define PORT_AR933X	99
 
+/* MCHP 16550A UART with 256 byte FIFOs */
+#define PORT_MCHP16550A	100
+
 /* ARC (Synopsys) on-chip UART */
 #define PORT_ARC       101
 
-- 
2.25.1

Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[Ilpo Järvinen]

On Sat, 1 Oct 2022, Kumaravel Thiagarajan wrote:

> pci1xxxx is a PCIe switch with a multi-function endpoint on one of its
> downstream ports. Quad-uart is one of the functions in the
> multi-function endpoint. This driver loads for the quad-uart and
> enumerates single or multiple instances of uart based on the PCIe
> subsystem device ID.
> 
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> ---
> Changes in v2:
> - Use only the 62.5 MHz for baud clock.
> - Define custom implementation for get_divisor and set_divisor.
> - Use BOTHER instead of UPF_SPD_CUST for non standard baud rates (untested).
> - Correct indentation in clock divisor computation.
> - Remove unnecessary call to pci_save_state in probe function.
> - Fix null pointer dereference in probe function.
> - Move pci1xxxx_rs485_config to a separate patch.
> - Depends on SERIAL_8250_PCI & default to SERIAL_8250.
> - Change PORT_MCHP16550A to 100 from 124.
> --- 
>  MAINTAINERS                             |   6 +
>  drivers/tty/serial/8250/8250_pci1xxxx.c | 394 ++++++++++++++++++++++++
>  drivers/tty/serial/8250/8250_port.c     |   8 +
>  drivers/tty/serial/8250/Kconfig         |  10 +
>  drivers/tty/serial/8250/Makefile        |   1 +
>  include/uapi/linux/serial_core.h        |   3 +
>  6 files changed, 422 insertions(+)
>  create mode 100644 drivers/tty/serial/8250/8250_pci1xxxx.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d30f26e07cd3..3390693d57ae 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -218,6 +218,12 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
>  F:	drivers/tty/serial/8250*
>  F:	include/linux/serial_8250.h
>  
> +MICROCHIP PCIe UART DRIVER
> +M:	Kumaravel Thiagarajan <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..41a4b94f52b4
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> @@ -0,0 +1,394 @@

> +#define PCI_VENDOR_ID_MCHP_PCI1XXXX	0x1055
> +
> +#define PCI_DEVICE_ID_MCHP_PCI12000	0xA002
> +#define PCI_DEVICE_ID_MCHP_PCI11010	0xA012
> +#define PCI_DEVICE_ID_MCHP_PCI11101	0xA022
> +#define PCI_DEVICE_ID_MCHP_PCI11400	0xA032
> +#define PCI_DEVICE_ID_MCHP_PCI11414	0xA042
> +
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p	0x0001
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p012	0x0002
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013	0x0003
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023	0x0004
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123	0x0005
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p01	0x0006
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02	0x0007
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03	0x0008
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12	0x0009
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13	0x000A
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23	0x000B
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p0	0x000C
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1	0x000D
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2	0x000E
> +#define PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3	0x000F
> +
> +#define PCI_SUBDEVICE_ID_MCHP_PCI12000	0xA002
> +#define PCI_SUBDEVICE_ID_MCHP_PCI11010	0xA012
> +#define PCI_SUBDEVICE_ID_MCHP_PCI11101	0xA022
> +#define PCI_SUBDEVICE_ID_MCHP_PCI11400	0xA032
> +#define PCI_SUBDEVICE_ID_MCHP_PCI11414	0xA042

Usually lowercase is used for hexadecimal letters.

> +#define UART_ACTV_REG			0x11
> +#define UART_ACTV_SET_ACTIVE		BIT(0)
> +
> +#define ADCL_CFG_REG			0x40
> +#define ADCL_CFG_POL_SEL		BIT(2)
> +#define ADCL_CFG_PIN_SEL		BIT(1)
> +#define ADCL_CFG_EN			BIT(0)
> +
> +#define CLK_SEL_REG			0x50
> +#define CLK_SEL_MASK			GENMASK(1, 0)
> +#define CLK_SEL_166MHZ			0x01
> +#define CLK_SEL_500MHZ			0x02

FIELD_PREP(CLK_SEL_MASK, ..) for thse two.

> +
> +#define CLK_DIVISOR_REG			0x54
> +
> +#define UART_PCI_CTRL_REG		0x80
> +#define UART_PCI_CTRL_SET_MULTIPLE_MSI	BIT(4)
> +#define UART_PCI_CTRL_D3_CLK_ENABLE	BIT(0)
> +
> +#define UART_WAKE_REG			0x8C
> +#define UART_WAKE_MASK_REG		0x90
> +#define UART_WAKE_N_PIN			BIT(2)
> +#define UART_WAKE_NCTS			BIT(1)
> +#define UART_WAKE_INT			BIT(0)
> +#define UART_WAKE_SRCS			(UART_WAKE_N_PIN | UART_WAKE_NCTS | UART_WAKE_INT)
> +
> +#define UART_RESET_REG			0x94
> +#define UART_RESET_D3_RESET_DISABLE	BIT(16)
> +
> +#define UART_BIT_SAMPLE_CNT 16
> +
> +struct pci1xxxx_8250 {
> +	struct pci_dev		*dev;
> +	unsigned int		nr;
> +	void __iomem		*membase;
> +	int			line[];
> +};

> +static unsigned int pci1xxxx_get_divisor(struct uart_port *port,
> +					 unsigned int baud,
> +					 unsigned int *frac)
> +{
> +	unsigned int quot;
> +
> +	/*
> +	 * Calculate baud rate sampling period in nano seconds.
> +	 * Fractional part x denotes x/255 parts of a nano second.
> +	 */
> +
> +	quot = ((1000000000) / (baud * UART_BIT_SAMPLE_CNT));
> +	*frac = (((1000000000 - (quot * baud * UART_BIT_SAMPLE_CNT)) /

NSEC_PER_SEC

> +		  UART_BIT_SAMPLE_CNT) * 255) / baud;
> +
> +	return quot;
> +}
> +
> +static void pci1xxxx_set_divisor(struct uart_port *port, unsigned int baud,
> +				 unsigned int quot, unsigned int frac)
> +{
> +	writel((quot << 8) | frac, (port->membase + CLK_DIVISOR_REG));

Define mask for quotient part and use FIELD_PREP().

Remove extra parenthesis.

> +static int pci1xxxx_serial_probe(struct pci_dev *dev,
> +				 const struct pci_device_id *ent)
> +{
> +	struct pci1xxxx_8250 *priv;
> +	struct uart_8250_port uart;
> +	unsigned int nr_ports, i;
> +	int num_vectors = 0;
> +	int rc;
> +
> +	rc = pcim_enable_device(dev);
> +	if (rc)
> +		return rc;
> +
> +	nr_ports = pci1xxxx_get_num_ports(dev);
> +
> +	priv = devm_kzalloc(&dev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->membase = pcim_iomap(dev, 0, 0);
> +	priv->dev = dev;
> +	priv->nr =  nr_ports;

Extra space.

> +
> +	pci_set_master(dev);
> +
> +	num_vectors  = pci_alloc_irq_vectors(dev, 1, 4, PCI_IRQ_ALL_TYPES);
> +	if (num_vectors < 0)
> +		return num_vectors;
> +
> +	memset(&uart, 0, sizeof(uart));
> +	uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_TYPE | UPF_FIXED_PORT;
> +	uart.port.uartclk = 62500000;
> +	uart.port.dev = &dev->dev;
> +
> +	if (num_vectors == 4)
> +		writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI, (priv->membase + UART_PCI_CTRL_REG));

Extra parenthesis.

-- 
 i.

Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[Andy Shevchenko]

On Sat, Oct 1, 2022 at 9:15 AM 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 an update, my comments below.

...

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

This doesn't look ordered at all. Please, locate your section in the
appropriate place.

...

> +#include <linux/serial_core.h>
> +#include <linux/8250_pci.h>
> +#include <asm/byteorder.h>
> +#include <linux/bitops.h>
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/pci.h>
> +#include <linux/tty.h>
> +#include <linux/io.h>

Please, keep this ordered and put a blank line between linux/* and
asm/* and "(local)" inclusions.

> +#include "8250.h"

...

> +#define PCI_VENDOR_ID_MCHP_PCI1XXXX    0x1055

Vendor should be vendor, this macro doesn't look right. See pci_ids.h
on how to properly define it.
Btw, PCI_VENDOR_ID_EFAR 0x1055 is there. Use it. (I believe microchip
bought that company in the past?)

...

> +#define PCI_DEVICE_ID_MCHP_PCI12000    0xA002
> +#define PCI_DEVICE_ID_MCHP_PCI11010    0xA012
> +#define PCI_DEVICE_ID_MCHP_PCI11101    0xA022
> +#define PCI_DEVICE_ID_MCHP_PCI11400    0xA032
> +#define PCI_DEVICE_ID_MCHP_PCI11414    0xA042

Use PCI_DEVICE_ID_#vendor_#device format. In a similar way for subdevice IDs.

...

> +#define UART_ACTV_REG                  0x11
> +#define UART_ACTV_SET_ACTIVE           BIT(0)

This namespace...

> +#define CLK_SEL_REG                    0x50
> +#define CLK_SEL_MASK                   GENMASK(1, 0)
> +#define CLK_SEL_166MHZ                 0x01
> +#define CLK_SEL_500MHZ                 0x02

> +#define CLK_DIVISOR_REG                        0x54

...and this one are potentially conflicting with more generic ones.

Ditto for the rest of the similar definitions.

...

> +static int pci1xxxx_setup_port(struct pci1xxxx_8250 *priv, struct uart_8250_port *port,
> +                              int offset)
> +{
> +       struct pci_dev *dev = priv->dev;
> +
> +       if (pci_resource_flags(dev, 0) & IORESOURCE_MEM) {
> +               if (!pcim_iomap(dev, 0, 0) && !pcim_iomap_table(dev))
> +                       return -ENOMEM;
> +
> +               port->port.iotype = UPIO_MEM;
> +               port->port.iobase = 0;
> +               port->port.mapbase = pci_resource_start(dev, 0) + offset;
> +               port->port.membase = pcim_iomap_table(dev)[0] + offset;
> +               port->port.regshift = 0;
> +       } else {
> +               port->port.iotype = UPIO_PORT;
> +               port->port.iobase = pci_resource_start(dev, 0) + offset;
> +               port->port.mapbase = 0;
> +               port->port.membase = NULL;
> +               port->port.regshift = 0;
> +       }
> +
> +       return 0;
> +}

Do you really have cards that are providing IO ports? If not, simplify
this accordingly.

...

> +       /*
> +        * Calculate baud rate sampling period in nano seconds.

nanoseconds

> +        * Fractional part x denotes x/255 parts of a nano second.
> +        */

...

> +       quot = ((1000000000) / (baud * UART_BIT_SAMPLE_CNT));
> +       *frac = (((1000000000 - (quot * baud * UART_BIT_SAMPLE_CNT)) /
> +                 UART_BIT_SAMPLE_CNT) * 255) / baud;

NSEC_PER_SEC ?

...

> +       writel((quot << 8) | frac, (port->membase + CLK_DIVISOR_REG));

Too many parentheses.

...

> +static int pci1xxxx_get_num_ports(struct pci_dev *dev)
> +{

> +       int nr_ports = 1;

Make this default case.

> +
> +       switch (dev->subsystem_device) {
> +       case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p0:
> +       case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1:
> +       case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2:
> +       case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3:
> +               nr_ports = 1;
> +               break;
> +       case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p01:
> +       case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02:
> +       case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03:
> +       case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12:
> +       case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13:
> +       case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23:
> +               nr_ports = 2;
> +               break;
> +       case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p012:
> +       case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123:
> +       case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013:
> +       case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023:
> +               nr_ports = 3;
> +               break;
> +       case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p:
> +       case PCI_SUBDEVICE_ID_MCHP_PCI11414:
> +               nr_ports = 4;
> +               break;
> +       }

> +       return nr_ports;

Drop the unnecessary local variable and use return directly from the
switch-cases.

> +}

...

> +       int first_offset = 0;

Use default switch-case.

...

> +       offset = first_offset + (idx * 256);

Too many parentheses. Check all your code for this kind of unnecessary.

...

> +       switch (priv->dev->subsystem_device) {

> +       case PCI_SUBDEVICE_ID_MCHP_PCI11414:
> +               uart->port.irq = pci_irq_vector(priv->dev, idx);
> +               break;

default?

> +       }

...

> +       uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_TYPE | UPF_FIXED_PORT;

Isn't there something duplicating here what's done in ->setup()?

...

> +       uart.port.uartclk = 62500000;

HZ_PER_MHZ ?

...

> +       if (num_vectors == 4)

This check should take care of all possible MSI >= 2, correct?

> +               writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI, (priv->membase + UART_PCI_CTRL_REG));
> +       else
> +               uart.port.irq = pci_irq_vector(dev, 0);

...

> +       for (i = 0; i < nr_ports; i++) {
> +               if (num_vectors == 4)

Ditto.

> +                       pci1xxxx_irq_assign(priv, &uart, i);
> +               priv->line[i] = -ENOSPC;
> +               rc = pci1xxxx_setup(priv, &uart, i);
> +               if (rc) {
> +                       dev_err(&dev->dev, "Failed to setup port %u\n", i);
> +                       break;
> +               }
> +               priv->line[i] = serial8250_register_8250_port(&uart);
> +
> +               if (priv->line[i] < 0) {
> +                       dev_err(&dev->dev,
> +                               "Couldn't register serial port %lx, irq %d, type %d, error %d\n",
> +                               uart.port.iobase, uart.port.irq,
> +                               uart.port.iotype, priv->line[i]);
> +                       break;
> +               }
> +       }

...

> +       [PORT_MCHP16550A] = {
> +               .name           = "MCHP16550A",
> +               .fifo_size      = 256,
> +               .tx_loadsz      = 256,
> +               .fcr            = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01,
> +               .rxtrig_bytes   = {2, 66, 130, 194},
> +               .flags          = UART_CAP_FIFO,
> +       },

Can you assign this in ->setup() or so instead of adding a new port type?

>  };

...

> +config SERIAL_8250_PCI1XXXX
> +       tristate "Microchip 8250 based serial port"
> +       depends on SERIAL_8250_PCI
> +       default SERIAL_8250
> +       help
> +        Select this option if you have a setup with Microchip PCIe
> +        Switch with serial port enabled and wish to enable 8250
> +        serial driver for the serial interface. This driver support
> +        will ensure to support baud rates upto 1.5Mpbs.

Keep it in the Quad UART group of config sections (Yes, I know that
not all of them are sorted there, but try your best).

...

> @@ -40,6 +40,7 @@ obj-$(CONFIG_SERIAL_8250_LPSS)                += 8250_lpss.o
>  obj-$(CONFIG_SERIAL_8250_MID)          += 8250_mid.o
>  obj-$(CONFIG_SERIAL_8250_PERICOM)      += 8250_pericom.o
>  obj-$(CONFIG_SERIAL_8250_PXA)          += 8250_pxa.o
> +obj-$(CONFIG_SERIAL_8250_PCI1XXXX)     += 8250_pci1xxxx.o
>  obj-$(CONFIG_SERIAL_8250_TEGRA)                += 8250_tegra.o
>  obj-$(CONFIG_SERIAL_8250_BCM7271)      += 8250_bcm7271.o
>  obj-$(CONFIG_SERIAL_OF_PLATFORM)       += 8250_of.o

Ditto.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Monday, October 3, 2022 2:53 PM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Subject: Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for
> quad-uart support.
> 
> On Sat, Oct 1, 2022 at 9:15 AM Kumaravel Thiagarajan
> <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 an update, my comments below.
Thank you for the review. I have started to review my code based on your comments and will get back to you soon.

Thank You.

Regards,
Kumar

Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[Tharunkumar.Pasumarthi]

On Mon, 2022-10-03 at 12:22 +0300, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> > +       return 0;
> > +}
> 
> Do you really have cards that are providing IO ports? If not, simplify
> this accordingly.

Device does not have IO ports. I will remove support in code.


> > +       if (num_vectors == 4)
> 
> This check should take care of all possible MSI >= 2, correct?

Hardware supports 2 modes:
1. One irq vector for all the instances
2. nth irq vector for nth instance
Some subsystem ID like PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p and
PCI_SUBDEVICE_ID_MCHP_PCI11414 will fail in 2nd mode if all the 4 irq vectors
are not assigned. Therefore, to reduce complexity, software will handle only 1
or 4 irq vectors. 

> > +       for (i = 0; i < nr_ports; i++) {
> > +               if (num_vectors == 4)
> 
> Ditto.

Hardware supports 2 modes:
1. One irq vector for all the instances
2. nth irq vector for nth instance
Some subsystem ID like PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p and
PCI_SUBDEVICE_ID_MCHP_PCI11414 will fail in 2nd mode if all the 4 irq vectors
are not assigned. Therefore, to reduce complexity, software will handle only 1
or 4 irq vectors. 

> > +               .flags          = UART_CAP_FIFO,
> > +       },
> 
> Can you assign this in ->setup() or so instead of adding a new port type?

Okay, I will assign in pci1xxxx_setup API.


Thanks Andy for the review. Going forward, I will be addressing the comments (if
any).

Thanks,
Tharun Kumar P

RE: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[Tharunkumar.Pasumarthi]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Monday, October 3, 2022 2:53 PM
> To: Kumaravel Thiagarajan - I21417
> <Kumaravel.Thiagarajan@microchip.com>
> Subject: Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for
> quad-uart support.
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > +       [PORT_MCHP16550A] = {
> > +               .name           = "MCHP16550A",
> > +               .fifo_size      = 256,
> > +               .tx_loadsz      = 256,
> > +               .fcr            = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01,
> > +               .rxtrig_bytes   = {2, 66, 130, 194},
> > +               .flags          = UART_CAP_FIFO,
> > +       },
> 
> Can you assign this in ->setup() or so instead of adding a new port type?

Hi Andy,
If I understand correctly, you suggest doing something like this inside pci1xxxx_setup() API:

pci1xxxx_setup(.., uart_8250_port *port, ..) {
..
port->port.fifosize = 256;
port->tx_loadsz = 256;
port->capabilities = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01;
..
}

instead of adding new port type PORT_MCHP16550A. But, if I do this, I cannot use sysfs interface for updating rx_trig_bytes right?
Kindly correct me if my understanding is wrong.

Thanks,
Tharun Kumar P

Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[Andy Shevchenko]

On Mon, Oct 31, 2022 at 11:24 AM <Tharunkumar.Pasumarthi@microchip.com> wrote:
> > -----Original Message-----
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Monday, October 3, 2022 2:53 PM

...

> > > +       [PORT_MCHP16550A] = {
> > > +               .name           = "MCHP16550A",
> > > +               .fifo_size      = 256,
> > > +               .tx_loadsz      = 256,
> > > +               .fcr            = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01,
> > > +               .rxtrig_bytes   = {2, 66, 130, 194},
> > > +               .flags          = UART_CAP_FIFO,
> > > +       },
> >
> > Can you assign this in ->setup() or so instead of adding a new port type?
>
> If I understand correctly, you suggest doing something like this inside pci1xxxx_setup() API:
>
> pci1xxxx_setup(.., uart_8250_port *port, ..) {
> ..
> port->port.fifosize = 256;
> port->tx_loadsz = 256;
> port->capabilities = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01;
> ..
> }
>
> instead of adding new port type PORT_MCHP16550A.

Yes, something like this.

> But, if I do this, I cannot use sysfs interface for updating rx_trig_bytes right?

Maybe, I don't remember by heart that part of the code. But why do you
need that ABI in the first place?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[Tharunkumar.Pasumarthi]

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Monday, October 31, 2022 8:07 PM
> To: Tharunkumar Pasumarthi - I67821
> <Tharunkumar.Pasumarthi@microchip.com>
> Subject: Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for
> quad-uart support.
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > But, if I do this, I cannot use sysfs interface for updating rx_trig_bytes right?
> 
> Maybe, I don't remember by heart that part of the code. But why do you
> need that ABI in the first place?

By using the sysfs interface, our driver will be able to update the trigger level for the receiver fifo interrupt at runtime.

Thanks,
Tharun Kumar P

Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[Andy Shevchenko]

On Tue, Nov 1, 2022 at 5:04 PM <Tharunkumar.Pasumarthi@microchip.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Monday, October 31, 2022 8:07 PM

...

> > > But, if I do this, I cannot use sysfs interface for updating rx_trig_bytes right?
> >
> > Maybe, I don't remember by heart that part of the code. But why do you
> > need that ABI in the first place?
>
> By using the sysfs interface, our driver will be able to update the trigger level for the receiver fifo interrupt at runtime.

This doesn't answer my question. What is this needed for?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Tuesday, November 1, 2022 8:47 PM
> To: Tharunkumar Pasumarthi - I67821
> <Tharunkumar.Pasumarthi@microchip.com>
> Subject: Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for
> quad-uart support.
>  
> On Tue, Nov 1, 2022 at 5:04 PM <Tharunkumar.Pasumarthi@microchip.com>
> wrote:
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Monday, October 31, 2022 8:07 PM
> 
> ...
> 
> > > > But, if I do this, I cannot use sysfs interface for updating rx_trig_bytes
> right?
> > >
> > > Maybe, I don't remember by heart that part of the code. But why do
> > > you need that ABI in the first place?
> >
> > By using the sysfs interface, our driver will be able to update the trigger
> level for the receiver fifo interrupt at runtime.
> 
> This doesn't answer my question. What is this needed for?
I think this will be useful for our customers in tuning the trigger level based on their application.
This is a UART based on programmed I/O and at higher speeds sometimes where there can be
continuous stream of data for a long time, we may need to set the trigger to the lower side to detect
early and avoid overflow.
On the other hand, in some applications where the continuous stream of data is not very long and
no risk of overflow, set it to higher side to avoid frequent interrupts..
We just want to keep this option open in the driver for different customers who would want to tune
this based on their application.
Please let us know if there are further questions or any issues in this approach.

Thank You.

Regards,
Kumar

Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[Christophe JAILLET]

Le 01/10/2022 à 08:15, Kumaravel Thiagarajan a écrit :
> pci1xxxx is a PCIe switch with a multi-function endpoint on one of its
> downstream ports. Quad-uart is one of the functions in the
> multi-function endpoint. This driver loads for the quad-uart and
> enumerates single or multiple instances of uart based on the PCIe
> subsystem device ID.
> 
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> ---

[...]

> +static int pci1xxxx_setup_port(struct pci1xxxx_8250 *priv, struct uart_8250_port *port,
> +			       int offset)
> +{
> +	struct pci_dev *dev = priv->dev;
> +
> +	if (pci_resource_flags(dev, 0) & IORESOURCE_MEM) {
> +		if (!pcim_iomap(dev, 0, 0) && !pcim_iomap_table(dev))
> +			return -ENOMEM;
> +
> +		port->port.iotype = UPIO_MEM;
> +		port->port.iobase = 0;
> +		port->port.mapbase = pci_resource_start(dev, 0) + offset;
> +		port->port.membase = pcim_iomap_table(dev)[0] + offset;

Hi,

Is it needed to call pcim_iomap_table(dev) twice? (here and a few lines 
above in the 'if')

> +		port->port.regshift = 0;
> +	} else {
> +		port->port.iotype = UPIO_PORT;
> +		port->port.iobase = pci_resource_start(dev, 0) + offset;
> +		port->port.mapbase = 0;
> +		port->port.membase = NULL;
> +		port->port.regshift = 0;
> +	}
> +
> +	return 0;
> +}

[...]

> +static int pci1xxxx_serial_probe(struct pci_dev *dev,
> +				 const struct pci_device_id *ent)
> +{
> +	struct pci1xxxx_8250 *priv;
> +	struct uart_8250_port uart;
> +	unsigned int nr_ports, i;
> +	int num_vectors = 0;
> +	int rc;
> +
> +	rc = pcim_enable_device(dev);
> +	if (rc)
> +		return rc;
> +
> +	nr_ports = pci1xxxx_get_num_ports(dev);
> +
> +	priv = devm_kzalloc(&dev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->membase = pcim_iomap(dev, 0, 0);
> +	priv->dev = dev;
> +	priv->nr =  nr_ports;
> +
> +	pci_set_master(dev);
> +
> +	num_vectors  = pci_alloc_irq_vectors(dev, 1, 4, PCI_IRQ_ALL_TYPES);
> +	if (num_vectors < 0)
> +		return num_vectors;
> +
> +	memset(&uart, 0, sizeof(uart));
> +	uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_TYPE | UPF_FIXED_PORT;
> +	uart.port.uartclk = 62500000;
> +	uart.port.dev = &dev->dev;
> +
> +	if (num_vectors == 4)
> +		writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI, (priv->membase + UART_PCI_CTRL_REG));
> +	else
> +		uart.port.irq = pci_irq_vector(dev, 0);
> +
> +	for (i = 0; i < nr_ports; i++) {
> +		if (num_vectors == 4)
> +			pci1xxxx_irq_assign(priv, &uart, i);
> +		priv->line[i] = -ENOSPC;
> +		rc = pci1xxxx_setup(priv, &uart, i);
> +		if (rc) {
> +			dev_err(&dev->dev, "Failed to setup port %u\n", i);
> +			break;
> +		}
> +		priv->line[i] = serial8250_register_8250_port(&uart);

In case of error, this should be undone in an error handling path in the 
probe, as done in the remove() function below.

If we break, we still continue and return success. But the last 
priv->line[i] are still 0. Is it an issue when pci1xxxx_serial_remove() 
is called?

> +
> +		if (priv->line[i] < 0) {
> +			dev_err(&dev->dev,
> +				"Couldn't register serial port %lx, irq %d, type %d, error %d\n",
> +				uart.port.iobase, uart.port.irq,
> +				uart.port.iotype, priv->line[i]);
> +			break;
> +		}
> +	}
> +
> +	pci_set_drvdata(dev, priv);
> +
> +	return 0;
> +}
> +
> +static void pci1xxxx_serial_remove(struct pci_dev *dev)
> +{
> +	struct pci1xxxx_8250 *priv = pci_get_drvdata(dev);
> +	int i;
> +
> +	for (i = 0; i < priv->nr; i++) {
> +		if (priv->line[i] >= 0)
> +			serial8250_unregister_port(priv->line[i]);
> +	}
> +}
> +

[...]

Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[Andy Shevchenko]

On Mon, Oct 3, 2022 at 10:36 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
> Le 01/10/2022 à 08:15, Kumaravel Thiagarajan a écrit :

...

> > +     if (pci_resource_flags(dev, 0) & IORESOURCE_MEM) {
> > +             if (!pcim_iomap(dev, 0, 0) && !pcim_iomap_table(dev))
> > +                     return -ENOMEM;
> > +
> > +             port->port.iotype = UPIO_MEM;
> > +             port->port.iobase = 0;
> > +             port->port.mapbase = pci_resource_start(dev, 0) + offset;
> > +             port->port.membase = pcim_iomap_table(dev)[0] + offset;

> Is it needed to call pcim_iomap_table(dev) twice? (here and a few lines
> above in the 'if')

Yes. But the main question I asked (if we really need IO ports
support) still remains.

> > +             port->port.regshift = 0;
> > +     } else {
> > +             port->port.iotype = UPIO_PORT;
> > +             port->port.iobase = pci_resource_start(dev, 0) + offset;
> > +             port->port.mapbase = 0;
> > +             port->port.membase = NULL;
> > +             port->port.regshift = 0;
> > +     }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[Tharunkumar.Pasumarthi]

On Mon, 2022-10-03 at 21:36 +0200, Christophe JAILLET wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> > +             }
> > +             priv->line[i] = serial8250_register_8250_port(&uart);
> 
> In case of error, this should be undone in an error handling path in the
> probe, as done in the remove() function below.
> 
> If we break, we still continue and return success. But the last
> priv->line[i] are still 0. Is it an issue when pci1xxxx_serial_remove()
> is called?

This will not cause a problem in pci1xxxx_serial_remove as this condition 'priv-
>line[i] >= 0' will be checked for each of the ports before calling
serial8250_unregister_port API.


Thanks,
Tharun Kumar P

Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[Christophe JAILLET]

Le 26/10/2022 à 13:12, Tharunkumar.Pasumarthi@microchip.com a écrit :
> On Mon, 2022-10-03 at 21:36 +0200, Christophe JAILLET wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>> content is safe
>>> +             }
>>> +             priv->line[i] = serial8250_register_8250_port(&uart);
>>
>> In case of error, this should be undone in an error handling path in the
>> probe, as done in the remove() function below.
>>
>> If we break, we still continue and return success. But the last
>> priv->line[i] are still 0. Is it an issue when pci1xxxx_serial_remove()
>> is called?
> 
> This will not cause a problem in pci1xxxx_serial_remove as this condition 'priv-
>> line[i] >= 0' will be checked for each of the ports before calling
> serial8250_unregister_port API.

Yes, this is my point.

We check for >=0 in pci1xxxx_serial_remove(), but if we have an error in 
the "for (i = 0; i < nr_ports; i++)" loop, some line[i] we'll still be 
zeroed because 'priv' is zalloc'ed.

In such a case, the probe still succeeds.

So, if pci1xxxx_serial_remove() is called later, we potentially call 
serial8250_unregister_port(0) several times.

This can lead to double (or more) free in serial8250_em485_destroy() 
(but maybe this path can't be taken here?) or maybe some other troubles 
elsewhere (I've not checked all the logic in uart_remove_one_port() to 
make sure that calling several times this function with the same args is 
fine).

So my point is maybe just a "can't happen" case.
If so, apologize for the noise.

CJ

> 
> 
> Thanks,
> Tharun Kumar P

[PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485 support to quad-uart driver.

[Kumaravel Thiagarajan]

pci1xxxx uart supports rs485 mode of operation in the hardware with
auto-direction control with configurable delay for releasing RTS after
the transmission. This patch adds support for the rs485 mode.

Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
---
Changes in v2:
- move pci1xxxx_rs485_config to a separate patch with
  pci1xxxx_rs485_supported.
---
 drivers/tty/serial/8250/8250_pci1xxxx.c | 57 +++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
index 41a4b94f52b4..999e5a284266 100644
--- a/drivers/tty/serial/8250/8250_pci1xxxx.c
+++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
@@ -137,6 +137,61 @@ static void pci1xxxx_set_divisor(struct uart_port *port, unsigned int baud,
 	writel((quot << 8) | frac, (port->membase + CLK_DIVISOR_REG));
 }
 
+static const struct serial_rs485 pci1xxxx_rs485_supported = {
+	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RTS_AFTER_SEND,
+	.delay_rts_after_send = 1,
+	/* Delay RTS before send is not supported */
+};
+
+static int pci1xxxx_rs485_config(struct uart_port *port,
+				 struct ktermios *termios,
+				 struct serial_rs485 *rs485)
+{
+	u32 clock_div = readl(port->membase + CLK_DIVISOR_REG);
+	u8 delay_in_baud_periods = 0;
+	u32 baud_period_in_ns = 0;
+	u8 data = 0;
+
+	/* pci1xxxx's uart hardware supports only RTS delay after
+	 * Tx and in units of bit times to a maximum of 15
+	 */
+
+	rs485->flags &= SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
+			SER_RS485_RTS_AFTER_SEND;
+
+	if (rs485->flags & SER_RS485_ENABLED) {
+		memset(rs485->padding, 0, sizeof(rs485->padding));
+
+		data = ADCL_CFG_EN | ADCL_CFG_PIN_SEL;
+
+		if (!(rs485->flags & SER_RS485_RTS_ON_SEND)) {
+			data |= ADCL_CFG_POL_SEL;
+			rs485->flags |=  SER_RS485_RTS_AFTER_SEND;
+		} else {
+			rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
+		}
+
+		if (rs485->delay_rts_after_send) {
+			baud_period_in_ns = ((clock_div >> 8) * 16);
+			delay_in_baud_periods = (rs485->delay_rts_after_send * 1000000) /
+						baud_period_in_ns;
+			if (delay_in_baud_periods > 0xF)
+				delay_in_baud_periods = 0xF;
+			data |= delay_in_baud_periods << 8;
+			rs485->delay_rts_after_send = (baud_period_in_ns * delay_in_baud_periods) /
+						      1000000;
+			rs485->delay_rts_before_send = 0;
+		}
+	} else {
+		memset(rs485, 0, sizeof(*rs485));
+	}
+
+	writeb(data, (port->membase + ADCL_CFG_REG));
+	port->rs485 = *rs485;
+
+	return 0;
+}
+
 static int pci1xxxx_get_num_ports(struct pci_dev *dev)
 {
 	int nr_ports = 1;
@@ -217,6 +272,8 @@ static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
 	port->port.set_termios = serial8250_do_set_termios;
 	port->port.get_divisor = pci1xxxx_get_divisor;
 	port->port.set_divisor = pci1xxxx_set_divisor;
+	port->port.rs485_config = pci1xxxx_rs485_config;
+	port->port.rs485_supported = pci1xxxx_rs485_supported;
 	ret = pci1xxxx_setup_port(priv, port, offset);
 	if (ret < 0)
 		return ret;
-- 
2.25.1

Re: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485 support to quad-uart driver.

[Ilpo Järvinen]

On Sat, 1 Oct 2022, Kumaravel Thiagarajan wrote:

> pci1xxxx uart supports rs485 mode of operation in the hardware with
> auto-direction control with configurable delay for releasing RTS after
> the transmission. This patch adds support for the rs485 mode.
> 
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> ---
> Changes in v2:
> - move pci1xxxx_rs485_config to a separate patch with
>   pci1xxxx_rs485_supported.
> ---
>  drivers/tty/serial/8250/8250_pci1xxxx.c | 57 +++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
> index 41a4b94f52b4..999e5a284266 100644
> --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> @@ -137,6 +137,61 @@ static void pci1xxxx_set_divisor(struct uart_port *port, unsigned int baud,
>  	writel((quot << 8) | frac, (port->membase + CLK_DIVISOR_REG));
>  }
>  
> +static const struct serial_rs485 pci1xxxx_rs485_supported = {
> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RTS_AFTER_SEND,
> +	.delay_rts_after_send = 1,
> +	/* Delay RTS before send is not supported */
> +};
> +
> +static int pci1xxxx_rs485_config(struct uart_port *port,
> +				 struct ktermios *termios,
> +				 struct serial_rs485 *rs485)
> +{
> +	u32 clock_div = readl(port->membase + CLK_DIVISOR_REG);
> +	u8 delay_in_baud_periods = 0;
> +	u32 baud_period_in_ns = 0;
> +	u8 data = 0;
> +
> +	/* pci1xxxx's uart hardware supports only RTS delay after
> +	 * Tx and in units of bit times to a maximum of 15
> +	 */
> +
> +	rs485->flags &= SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> +			SER_RS485_RTS_AFTER_SEND;

Drop this, core handles it for you.

I think I already mentioned to you that there is a lot of stuff that core 
handles for you.

> +	if (rs485->flags & SER_RS485_ENABLED) {
> +		memset(rs485->padding, 0, sizeof(rs485->padding));

Core handles this for you.

> +		data = ADCL_CFG_EN | ADCL_CFG_PIN_SEL;
> +
> +		if (!(rs485->flags & SER_RS485_RTS_ON_SEND)) {
> +			data |= ADCL_CFG_POL_SEL;
> +			rs485->flags |=  SER_RS485_RTS_AFTER_SEND;
> +		} else {
> +			rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
> +		}

Core handles that flags sanitization for you.

> +		if (rs485->delay_rts_after_send) {
> +			baud_period_in_ns = ((clock_div >> 8) * 16);

If that >> 8 is there to take part of the CLK_DIVISOR_REG register's bits, 
you want to define a mask and use FIELD_GET().

> +			delay_in_baud_periods = (rs485->delay_rts_after_send * 1000000) /

NSEC_PER_MSEC?

> +						baud_period_in_ns;
> +			if (delay_in_baud_periods > 0xF)
> +				delay_in_baud_periods = 0xF;
> +			data |= delay_in_baud_periods << 8;

You want to add something along these lines:
#define ADCL_CFG_RTS_DELAY_MASK		GENMASK(11, 8)

Then do:
			delay_in_baud_periods = min(delay_in_baud_periods, 
						    FIELD_MAX(ADCL_CFG_RTS_DELAY_MASK));
			data |= FIELD_PREP(ADCL_CFG_RTS_DELAY_MASK, delay_in_baud_periods);

> +			rs485->delay_rts_after_send = (baud_period_in_ns * delay_in_baud_periods) /
> +						      1000000;

NSEC_PER_MSEC?

> +			rs485->delay_rts_before_send = 0;
> +		}
> +	} else {
> +		memset(rs485, 0, sizeof(*rs485));

Core handles this.

> +	}
> +
> +	writeb(data, (port->membase + ADCL_CFG_REG));
> +	port->rs485 = *rs485;

Core handles this.

> +	return 0;
> +}
> +
>  static int pci1xxxx_get_num_ports(struct pci_dev *dev)
>  {
>  	int nr_ports = 1;
> @@ -217,6 +272,8 @@ static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
>  	port->port.set_termios = serial8250_do_set_termios;
>  	port->port.get_divisor = pci1xxxx_get_divisor;
>  	port->port.set_divisor = pci1xxxx_set_divisor;
> +	port->port.rs485_config = pci1xxxx_rs485_config;
> +	port->port.rs485_supported = pci1xxxx_rs485_supported;
>  	ret = pci1xxxx_setup_port(priv, port, offset);
>  	if (ret < 0)
>  		return ret;
> 

-- 
 i.

RE: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485 support to quad-uart driver.

[Tharunkumar.Pasumarthi]

> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: Monday, October 3, 2022 2:34 PM
> To: Kumaravel Thiagarajan - I21417
> <Kumaravel.Thiagarajan@microchip.com>
> Subject: Re: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485
> support to quad-uart driver.
> 
> [Some people who received this message don't often get email from
> ilpo.jarvinen@linux.intel.com. Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > +     if (rs485->flags & SER_RS485_ENABLED) {
> > +             memset(rs485->padding, 0, sizeof(rs485->padding));
> 
> Core handles this for you.

I went through the code and it seems like this is not taken care by the core.
Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.

> > +             if (!(rs485->flags & SER_RS485_RTS_ON_SEND)) {
> > +                     data |= ADCL_CFG_POL_SEL;
> > +                     rs485->flags |=  SER_RS485_RTS_AFTER_SEND;
> > +             } else {
> > +                     rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
> > +             }
> 
> Core handles that flags sanitization for you.

I went through the code and it seems like this is not taken care by the core.
Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.

> > +     } else {
> > +             memset(rs485, 0, sizeof(*rs485));
> 
> Core handles this.

I went through the code and it seems like this is not taken care by the core.
Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.

> > +     writeb(data, (port->membase + ADCL_CFG_REG));
> > +     port->rs485 = *rs485;
> 
> Core handles this.

I went through the code and it seems like this is not taken care by the core.
Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.


Thanks,
Tharun Kumar P

RE: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485 support to quad-uart driver.

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2778 bytes --]

On Tue, 1 Nov 2022, Tharunkumar.Pasumarthi@microchip.com wrote:

> > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Sent: Monday, October 3, 2022 2:34 PM
> > To: Kumaravel Thiagarajan - I21417
> > <Kumaravel.Thiagarajan@microchip.com>
> > Subject: Re: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485
> > support to quad-uart driver.
> > 
> > [Some people who received this message don't often get email from
> > ilpo.jarvinen@linux.intel.com. Learn why this is important at
> > https://aka.ms/LearnAboutSenderIdentification ]
> > 
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> > 
> > > +     if (rs485->flags & SER_RS485_ENABLED) {
> > > +             memset(rs485->padding, 0, sizeof(rs485->padding));
> > 
> > Core handles this for you.
> 
> I went through the code and it seems like this is not taken care by the core.
> Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
> This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.
> 
> > > +             if (!(rs485->flags & SER_RS485_RTS_ON_SEND)) {
> > > +                     data |= ADCL_CFG_POL_SEL;
> > > +                     rs485->flags |=  SER_RS485_RTS_AFTER_SEND;
> > > +             } else {
> > > +                     rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
> > > +             }
> > 
> > Core handles that flags sanitization for you.
> 
> I went through the code and it seems like this is not taken care by the core.
> Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
> This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.
> 
> > > +     } else {
> > > +             memset(rs485, 0, sizeof(*rs485));
> > 
> > Core handles this.
> 
> I went through the code and it seems like this is not taken care by the core.
> Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
> This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.
> 
> > > +     writeb(data, (port->membase + ADCL_CFG_REG));
> > > +     port->rs485 = *rs485;
> > 
> > Core handles this.
> 
> I went through the code and it seems like this is not taken care by the core.
> Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
> This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.

It has nothing to do with serial8250_em485_config.

It is very hard to believe you couldn't find 
uart_sanitize_serial_rs485() and uart_set_rs485_config() yourself, the 
latter calls your driver specific rs485 handler.

-- 
 i.

Re: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485 support to quad-uart driver.

[Andy Shevchenko]

On Tue, Nov 1, 2022 at 5:25 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
> On Tue, 1 Nov 2022, Tharunkumar.Pasumarthi@microchip.com wrote:

...

> > I went through the code and it seems like this is not taken care by the core.
> > Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
> > This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.
>
> It has nothing to do with serial8250_em485_config.
>
> It is very hard to believe you couldn't find
> uart_sanitize_serial_rs485() and uart_set_rs485_config() yourself, the
> latter calls your driver specific rs485 handler.

Which version has this API? If it's v6.1-rc1 and patches are made
against v6.0, it's possible to miss something.

In any case, the patches to the serial subsystem should always be done
against the tty/tty-next branch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485 support to quad-uart driver.

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]

On Tue, 1 Nov 2022, Andy Shevchenko wrote:

> On Tue, Nov 1, 2022 at 5:25 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> > On Tue, 1 Nov 2022, Tharunkumar.Pasumarthi@microchip.com wrote:
> 
> ...
> 
> > > I went through the code and it seems like this is not taken care by the core.
> > > Do you suggest calling 'serial8250_em485_config' inside 'pci1xxxx_rs485_config' callback?
> > > This has not been done since we do not need all the configurations done inside 'serial8250_em485_config'.
> >
> > It has nothing to do with serial8250_em485_config.
> >
> > It is very hard to believe you couldn't find
> > uart_sanitize_serial_rs485() and uart_set_rs485_config() yourself, the
> > latter calls your driver specific rs485 handler.
> 
> Which version has this API? If it's v6.1-rc1 and patches are made
> against v6.0, it's possible to miss something.
> 
> In any case, the patches to the serial subsystem should always be done
> against the tty/tty-next branch.

It has been for multiple stable kernel version already. It was moved to 
own function by this commit:

git describe --contains 2dbd0c14ebe8836eaf890c7f50f3fc5d26d67d95
v6.0-rc1~64^2~129

Originally introduced here:
git describe --contains 0ed12afa5655512ee418047fb3546d229df20aa1
v5.19-rc1~47^2~142

There have been perhaps one of those things I pointed out that was added 
later than the others but it won't explain why nothing was found from the 
code.


-- 
 i.

RE: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485 support to quad-uart driver.

[Tharunkumar.Pasumarthi]

> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: Tuesday, November 1, 2022 9:20 PM
> To: Andy Shevchenko <andy.shevchenko@gmail.com>
> Subject: Re: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485
> support to quad-uart driver.
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>  
> There have been perhaps one of those things I pointed out that was added
> later than the others but it won't explain why nothing was found from the
> code.

Sorry Ilpo. This was a mistake from my side. I was referring to older kernel instead of the one in tty-next branch by mistake. I will make sure not to repeat this going forward.

Thanks,
Tharun Kumar P

Re: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485 support to quad-uart driver.

[Ilpo Järvinen]

On Sat, 1 Oct 2022, Kumaravel Thiagarajan wrote:

> pci1xxxx uart supports rs485 mode of operation in the hardware with
> auto-direction control with configurable delay for releasing RTS after
> the transmission. This patch adds support for the rs485 mode.
> 
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> ---
> Changes in v2:
> - move pci1xxxx_rs485_config to a separate patch with
>   pci1xxxx_rs485_supported.
> ---
>  drivers/tty/serial/8250/8250_pci1xxxx.c | 57 +++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
> index 41a4b94f52b4..999e5a284266 100644
> --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> +
> +		if (rs485->delay_rts_after_send) {
> +			baud_period_in_ns = ((clock_div >> 8) * 16);

Is this 16 perhaps UART_BIT_SAMPLE_CNT?


-- 
 i.

RE: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485 support to quad-uart driver.

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: Monday, October 3, 2022 2:51 PM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Subject: Re: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485
> support to quad-uart driver.
> 
> On Sat, 1 Oct 2022, Kumaravel Thiagarajan wrote:
> 
> > pci1xxxx uart supports rs485 mode of operation in the hardware with
> > auto-direction control with configurable delay for releasing RTS after
> > the transmission. This patch adds support for the rs485 mode.
> >
> > Signed-off-by: Kumaravel Thiagarajan
> > <kumaravel.thiagarajan@microchip.com>
> > ---
> > Changes in v2:
> > - move pci1xxxx_rs485_config to a separate patch with
> >   pci1xxxx_rs485_supported.
> > ---
> >  drivers/tty/serial/8250/8250_pci1xxxx.c | 57
> > +++++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > index 41a4b94f52b4..999e5a284266 100644
> > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > +
> > +             if (rs485->delay_rts_after_send) {
> > +                     baud_period_in_ns = ((clock_div >> 8) * 16);
> 
> Is this 16 perhaps UART_BIT_SAMPLE_CNT?
Yes. Is there any macro definition for that? I could not find any definition in the above name.

Thank You.

Regards,
Kumar

RE: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485 support to quad-uart driver.

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1615 bytes --]

On Tue, 4 Oct 2022, Kumaravel.Thiagarajan@microchip.com wrote:

> > -----Original Message-----
> > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Sent: Monday, October 3, 2022 2:51 PM
> > To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> > Subject: Re: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485
> > support to quad-uart driver.
> > 
> > On Sat, 1 Oct 2022, Kumaravel Thiagarajan wrote:
> > 
> > > pci1xxxx uart supports rs485 mode of operation in the hardware with
> > > auto-direction control with configurable delay for releasing RTS after
> > > the transmission. This patch adds support for the rs485 mode.
> > >
> > > Signed-off-by: Kumaravel Thiagarajan
> > > <kumaravel.thiagarajan@microchip.com>
> > > ---
> > > Changes in v2:
> > > - move pci1xxxx_rs485_config to a separate patch with
> > >   pci1xxxx_rs485_supported.
> > > ---
> > >  drivers/tty/serial/8250/8250_pci1xxxx.c | 57
> > > +++++++++++++++++++++++++
> > >  1 file changed, 57 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > > b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > > index 41a4b94f52b4..999e5a284266 100644
> > > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > > +
> > > +             if (rs485->delay_rts_after_send) {
> > > +                     baud_period_in_ns = ((clock_div >> 8) * 16);
> > 
> > Is this 16 perhaps UART_BIT_SAMPLE_CNT?
> Yes. Is there any macro definition for that? I could not find any 
> definition in the above name. 

You're adding it in your 1/3 patch :-).

-- 
 i.

[PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power management functions to 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 on resume.

Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
---
Changes in v2:
- Use DEFINE_SIMPLE_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS.
- Use pm_sleep_ptr instead of CONFIG_PM_SLEEP.
- Change the return data type of pci1xxxx_port_suspend to bool from int.
---
 drivers/tty/serial/8250/8250_pci1xxxx.c | 112 ++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
index 999e5a284266..0a0459f66177 100644
--- a/drivers/tty/serial/8250/8250_pci1xxxx.c
+++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
@@ -352,6 +352,112 @@ static void pci1xxxx_irq_assign(struct pci1xxxx_8250 *priv,
 	}
 }
 
+static bool pci1xxxx_port_suspend(int line)
+{
+	struct uart_8250_port *up = serial8250_get_port(line);
+	struct uart_port *port = &up->port;
+	unsigned long flags;
+	u8 wakeup_mask;
+	bool ret = false;
+
+	if (port->suspended == 0 && port->dev) {
+		wakeup_mask = readb(up->port.membase + UART_WAKE_MASK_REG);
+
+		spin_lock_irqsave(&port->lock, flags);
+		port->mctrl &= ~TIOCM_OUT2;
+		port->ops->set_mctrl(port, port->mctrl);
+		spin_unlock_irqrestore(&port->lock, flags);
+
+		if ((wakeup_mask & UART_WAKE_SRCS) != UART_WAKE_SRCS)
+			ret = true;
+	}
+
+	writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
+
+	return ret;
+}
+
+static void pci1xxxx_port_resume(int line)
+{
+	struct uart_8250_port *up = serial8250_get_port(line);
+	struct uart_port *port = &up->port;
+	unsigned long flags;
+
+	writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
+
+	if (port->suspended == 0) {
+		spin_lock_irqsave(&port->lock, flags);
+		port->mctrl |= TIOCM_OUT2;
+		port->ops->set_mctrl(port, port->mctrl);
+		spin_unlock_irqrestore(&port->lock, flags);
+	}
+}
+
+static int pci1xxxx_suspend(struct device *dev)
+{
+	struct pci1xxxx_8250 *priv = dev_get_drvdata(dev);
+	struct pci_dev *pcidev = to_pci_dev(dev);
+	unsigned int data;
+	void __iomem *p;
+	bool wakeup = false;
+	int i;
+
+	for (i = 0; i < priv->nr; i++) {
+		if (priv->line[i] >= 0) {
+			serial8250_suspend_port(priv->line[i]);
+			wakeup |= pci1xxxx_port_suspend(priv->line[i]);
+		}
+	}
+
+	p = pci_ioremap_bar(pcidev, 0);
+	if (!p) {
+		dev_err(dev, "remapping of bar 0 memory failed");
+		return -ENOMEM;
+	}
+
+	data = readl(p + UART_RESET_REG);
+	writel(data | UART_RESET_D3_RESET_DISABLE, p + UART_RESET_REG);
+
+	if (wakeup)
+		writeb(UART_PCI_CTRL_D3_CLK_ENABLE, (p + UART_PCI_CTRL_REG));
+
+	iounmap(p);
+
+	device_set_wakeup_enable(dev, true);
+
+	pci_wake_from_d3(pcidev, true);
+
+	return 0;
+}
+
+static int pci1xxxx_resume(struct device *dev)
+{
+	struct pci1xxxx_8250 *priv = dev_get_drvdata(dev);
+	struct pci_dev *pcidev = to_pci_dev(dev);
+	unsigned int data;
+	void __iomem *p;
+	int i;
+
+	p = pci_ioremap_bar(pcidev, 0);
+	if (!p) {
+		dev_err(dev, "remapping of bar 0 memory failed");
+		return -ENOMEM;
+	}
+
+	data = readl(p + UART_RESET_REG);
+	writel(data & ~UART_RESET_D3_RESET_DISABLE, p + UART_RESET_REG);
+	iounmap(p);
+
+	for (i = 0; i < priv->nr; i++) {
+		if (priv->line[i] >= 0) {
+			pci1xxxx_port_resume(priv->line[i]);
+			serial8250_resume_port(priv->line[i]);
+		}
+	}
+
+	return 0;
+}
+
 static int pci1xxxx_serial_probe(struct pci_dev *dev,
 				 const struct pci_device_id *ent)
 {
@@ -427,6 +533,9 @@ static void pci1xxxx_serial_remove(struct pci_dev *dev)
 	}
 }
 
+static DEFINE_SIMPLE_DEV_PM_OPS(pci1xxxx_pm_ops, pci1xxxx_suspend,
+				pci1xxxx_resume);
+
 static const struct pci_device_id pci1xxxx_pci_tbl[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11010) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11101) },
@@ -441,6 +550,9 @@ static struct pci_driver pci1xxxx_pci_driver = {
 	.name		= "pci1xxxx serial",
 	.probe		= pci1xxxx_serial_probe,
 	.remove		= pci1xxxx_serial_remove,
+	.driver	= {
+		.pm     = pm_sleep_ptr(&pci1xxxx_pm_ops),
+	},
 	.id_table	= pci1xxxx_pci_tbl,
 };
 
-- 
2.25.1

Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power management functions to quad-uart driver.

[Andy Shevchenko]

On Sat, Oct 1, 2022 at 9:15 AM 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 on resume.

...

> +       port->suspended == 0

How is this check race-protected?

...

> +static void pci1xxxx_port_resume(int line)
> +{

> +       if (port->suspended == 0) {

Ditto.

> +       }
> +}

...

If you have similarities with 8250_pci, probably you need to split it
to 8250_pcilib.c and share. (See how 8250_dw /8250_lpss are done in
that sense.)

--
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power management functions to quad-uart driver.

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Monday, October 3, 2022 2:57 PM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Subject: Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power
> management functions to quad-uart driver.
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Sat, Oct 1, 2022 at 9:15 AM Kumaravel Thiagarajan
> <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 on resume.
> 
> ...
> 
> > +       port->suspended == 0
> 
> How is this check race-protected?
> 
> ...
> 
> > +static void pci1xxxx_port_resume(int line) {
> 
> > +       if (port->suspended == 0) {
> 
> Ditto.
> 
> > +       }
> > +}
> 
> ...
> 
> If you have similarities with 8250_pci, probably you need to split it to
> 8250_pcilib.c and share. (See how 8250_dw /8250_lpss are done in that
> sense.)
I will review my code against all the above comments and get back to you.

Thank You.

Regards,
Kumar

Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power management functions to quad-uart driver.

[Tharunkumar.Pasumarthi]

On Mon, 2022-10-03 at 12:26 +0300, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> > +       port->suspended == 0
> 
> How is this check race-protected?

I will use mutex_lock to handle race condition.


Thanks,
Tharun Kumar P

RE: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power management functions to quad-uart driver.

[Tharunkumar.Pasumarthi]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Monday, October 3, 2022 2:57 PM
> To: Kumaravel Thiagarajan - I21417
> <Kumaravel.Thiagarajan@microchip.com>
> Subject: Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power
> management functions to quad-uart driver.
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>  
> If you have similarities with 8250_pci, probably you need to split it to
> 8250_pcilib.c and share. (See how 8250_dw /8250_lpss are done in that
> sense.)

Functions related to suspend and resume use registers specific to our IP which other drivers cannot use. I will move functions like pci1xxxx_set_divisor, pci1xxxx_get_divisor, pci1xxxx_setup_port and pci1xxxx_rs485_config to a new file.

Thanks,
Tharun Kumar P

RE: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power management functions to quad-uart driver.

[Tharunkumar.Pasumarthi]

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Monday, October 3, 2022 2:57 PM
> To: Kumaravel Thiagarajan - I21417
> <Kumaravel.Thiagarajan@microchip.com>
> Subject: Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power
> management functions to quad-uart driver.
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>  
> If you have similarities with 8250_pci, probably you need to split it to
> 8250_pcilib.c and share. (See how 8250_dw /8250_lpss are done in that
> sense.)

Hi Andy,
All the functions used in 8250_pci1xxxx.c  that have similarity with 8250_pci use registers
that are specific to our IP.  The only function that can be moved to common library is the 
setup_port. But, for that the first argument of setup_port must be changed to 
'struct pci_dev *dev' (priv->dev).  Do you suggest doing this?

Thanks,
Tharun Kumar P

Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power management functions to quad-uart driver.

[Andy Shevchenko]

On Fri, Nov 4, 2022 at 12:23 PM <Tharunkumar.Pasumarthi@microchip.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Monday, October 3, 2022 2:57 PM

> > If you have similarities with 8250_pci, probably you need to split it to
> > 8250_pcilib.c and share. (See how 8250_dw /8250_lpss are done in that
> > sense.)
>
> All the functions used in 8250_pci1xxxx.c  that have similarity with 8250_pci use registers
> that are specific to our IP.  The only function that can be moved to common library is the
> setup_port. But, for that the first argument of setup_port must be changed to
> 'struct pci_dev *dev' (priv->dev).  Do you suggest doing this?

So, you can create a common serial8250_setup_port(struct pci_dev *dev,
...) and call it from the static setup_port() inside 8250_pci.c. This
way you won't introduce too many churn.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power management functions to quad-uart driver.

[Ilpo Järvinen]

On Sat, 1 Oct 2022, Kumaravel Thiagarajan wrote:

> pci1xxxx's quad-uart function has the capability to wake up the host from
> suspend state. Enable wakeup before entering into suspend and disable
> wakeup on resume.
> 
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> ---
> Changes in v2:
> - Use DEFINE_SIMPLE_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS.
> - Use pm_sleep_ptr instead of CONFIG_PM_SLEEP.
> - Change the return data type of pci1xxxx_port_suspend to bool from int.
> ---
>  drivers/tty/serial/8250/8250_pci1xxxx.c | 112 ++++++++++++++++++++++++
>  1 file changed, 112 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
> index 999e5a284266..0a0459f66177 100644
> --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> @@ -352,6 +352,112 @@ static void pci1xxxx_irq_assign(struct pci1xxxx_8250 *priv,
>  	}
>  }
>  
> +static bool pci1xxxx_port_suspend(int line)
> +{
> +	struct uart_8250_port *up = serial8250_get_port(line);
> +	struct uart_port *port = &up->port;
> +	unsigned long flags;
> +	u8 wakeup_mask;
> +	bool ret = false;
> +
> +	if (port->suspended == 0 && port->dev) {
> +		wakeup_mask = readb(up->port.membase + UART_WAKE_MASK_REG);
> +
> +		spin_lock_irqsave(&port->lock, flags);
> +		port->mctrl &= ~TIOCM_OUT2;
> +		port->ops->set_mctrl(port, port->mctrl);
> +		spin_unlock_irqrestore(&port->lock, flags);
> +
> +		if ((wakeup_mask & UART_WAKE_SRCS) != UART_WAKE_SRCS)
> +			ret = true;
> +	}
> +
> +	writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> +
> +	return ret;
> +}
> +
> +static void pci1xxxx_port_resume(int line)
> +{
> +	struct uart_8250_port *up = serial8250_get_port(line);
> +	struct uart_port *port = &up->port;
> +	unsigned long flags;
> +
> +	writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> +
> +	if (port->suspended == 0) {

Is this check the right way around?

> +		spin_lock_irqsave(&port->lock, flags);
> +		port->mctrl |= TIOCM_OUT2;
> +		port->ops->set_mctrl(port, port->mctrl);
> +		spin_unlock_irqrestore(&port->lock, flags);
> +	}
> +}
> +
> +static int pci1xxxx_suspend(struct device *dev)
> +{
> +	struct pci1xxxx_8250 *priv = dev_get_drvdata(dev);
> +	struct pci_dev *pcidev = to_pci_dev(dev);
> +	unsigned int data;
> +	void __iomem *p;
> +	bool wakeup = false;
> +	int i;
> +
> +	for (i = 0; i < priv->nr; i++) {
> +		if (priv->line[i] >= 0) {
> +			serial8250_suspend_port(priv->line[i]);
> +			wakeup |= pci1xxxx_port_suspend(priv->line[i]);

So first serial8250_suspend_port() calls into uart_suspend_port() that
sets port->suspended to 1, then pci1xxxx_port_suspend() checks if it's 0.
Is this intentional?


-- 
 i.

RE: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power management functions to quad-uart driver.

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: Monday, October 3, 2022 3:21 PM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Subject: Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power
> management functions to quad-uart driver.
>  
> On Sat, 1 Oct 2022, Kumaravel Thiagarajan wrote:
> 
> > pci1xxxx's quad-uart function has the capability to wake up the host
> > from suspend state. Enable wakeup before entering into suspend and
> > disable wakeup on resume.
> >
> > Signed-off-by: Kumaravel Thiagarajan
> > <kumaravel.thiagarajan@microchip.com>
> > ---
> > Changes in v2:
> > - Use DEFINE_SIMPLE_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS.
> > - Use pm_sleep_ptr instead of CONFIG_PM_SLEEP.
> > - Change the return data type of pci1xxxx_port_suspend to bool from int.
> > ---
> >  drivers/tty/serial/8250/8250_pci1xxxx.c | 112
> > ++++++++++++++++++++++++
> >  1 file changed, 112 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > index 999e5a284266..0a0459f66177 100644
> > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > @@ -352,6 +352,112 @@ static void pci1xxxx_irq_assign(struct
> pci1xxxx_8250 *priv,
> >       }
> >  }
> >
> > +static bool pci1xxxx_port_suspend(int line) {
> > +     struct uart_8250_port *up = serial8250_get_port(line);
> > +     struct uart_port *port = &up->port;
> > +     unsigned long flags;
> > +     u8 wakeup_mask;
> > +     bool ret = false;
> > +
> > +     if (port->suspended == 0 && port->dev) {
> > +             wakeup_mask = readb(up->port.membase +
> > + UART_WAKE_MASK_REG);
> > +
> > +             spin_lock_irqsave(&port->lock, flags);
> > +             port->mctrl &= ~TIOCM_OUT2;
> > +             port->ops->set_mctrl(port, port->mctrl);
> > +             spin_unlock_irqrestore(&port->lock, flags);
> > +
> > +             if ((wakeup_mask & UART_WAKE_SRCS) != UART_WAKE_SRCS)
> > +                     ret = true;
> > +     }
> > +
> > +     writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> > +
> > +     return ret;
> > +}
> > +
> > +static void pci1xxxx_port_resume(int line) {
> > +     struct uart_8250_port *up = serial8250_get_port(line);
> > +     struct uart_port *port = &up->port;
> > +     unsigned long flags;
> > +
> > +     writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> > +
> > +     if (port->suspended == 0) {
> 
> Is this check the right way around?
Yes. I think port->suspended is not set for wake-up capable ports and the code in this if block gets executed for those ports.
I will check this again.
> 
> > +             spin_lock_irqsave(&port->lock, flags);
> > +             port->mctrl |= TIOCM_OUT2;
> > +             port->ops->set_mctrl(port, port->mctrl);
> > +             spin_unlock_irqrestore(&port->lock, flags);
> > +     }
> > +}
> > +
> > +static int pci1xxxx_suspend(struct device *dev) {
> > +     struct pci1xxxx_8250 *priv = dev_get_drvdata(dev);
> > +     struct pci_dev *pcidev = to_pci_dev(dev);
> > +     unsigned int data;
> > +     void __iomem *p;
> > +     bool wakeup = false;
> > +     int i;
> > +
> > +     for (i = 0; i < priv->nr; i++) {
> > +             if (priv->line[i] >= 0) {
> > +                     serial8250_suspend_port(priv->line[i]);
> > +                     wakeup |= pci1xxxx_port_suspend(priv->line[i]);
> 
> So first serial8250_suspend_port() calls into uart_suspend_port() that sets
> port->suspended to 1, then pci1xxxx_port_suspend() checks if it's 0.
> Is this intentional?
Yes. I think port->suspended does not seem to be set for wake-up capable ports and only 
for those ports, inside pci1xxxx_port_suspend, TIOCM_OUT2 is cleared.
But I must check for the race condition as Andy had pointed out.
Please let me know if there are any questions.

Thank You.

Regards,
Kumar