* [PATCH v1 tty-next 0/2] 8250: microchip: pci1xxxx: Add driver for the pci1xxxx's quad-uart function.
@ 2022-08-30 18:00 Kumaravel Thiagarajan
2022-08-30 18:00 ` [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
2022-08-30 18:00 ` [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver Kumaravel Thiagarajan
0 siblings, 2 replies; 21+ messages in thread
From: Kumaravel Thiagarajan @ 2022-08-30 18:00 UTC (permalink / raw)
To: gregkh, jirislaby, ilpo.jarvinen, andy.shevchenko,
u.kleine-koenig, johan, wander, etremblay, macro, geert+renesas,
jk, phil.edworthy, lukas
Cc: linux-kernel, linux-serial, UNGLinuxDriver
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v1 tty-next 1/2] 8250: microchip: pci1xxxx: Add driver for the quad-uart function in the multi-function endpoint of pci1xxxx device.
2022-08-30 18:00 [PATCH v1 tty-next 0/2] 8250: microchip: pci1xxxx: Add driver for the pci1xxxx's quad-uart function Kumaravel Thiagarajan
@ 2022-08-30 18:00 ` Kumaravel Thiagarajan
2022-08-30 19:53 ` Andy Shevchenko
` (4 more replies)
2022-08-30 18:00 ` [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver Kumaravel Thiagarajan
1 sibling, 5 replies; 21+ messages in thread
From: Kumaravel Thiagarajan @ 2022-08-30 18:00 UTC (permalink / raw)
To: gregkh, jirislaby, ilpo.jarvinen, andy.shevchenko,
u.kleine-koenig, johan, wander, etremblay, macro, geert+renesas,
jk, phil.edworthy, lukas
Cc: linux-kernel, linux-serial, UNGLinuxDriver
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver.
2022-08-30 18:00 [PATCH v1 tty-next 0/2] 8250: microchip: pci1xxxx: Add driver for the pci1xxxx's quad-uart function Kumaravel Thiagarajan
2022-08-30 18:00 ` [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
@ 2022-08-30 18:00 ` Kumaravel Thiagarajan
2022-08-30 19:56 ` Andy Shevchenko
` (2 more replies)
1 sibling, 3 replies; 21+ messages in thread
From: Kumaravel Thiagarajan @ 2022-08-30 18:00 UTC (permalink / raw)
To: gregkh, jirislaby, ilpo.jarvinen, andy.shevchenko,
u.kleine-koenig, johan, wander, etremblay, macro, geert+renesas,
jk, phil.edworthy, lukas
Cc: linux-kernel, linux-serial, UNGLinuxDriver
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* 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.
2022-08-30 18:00 ` [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
@ 2022-08-30 19:53 ` Andy Shevchenko
2022-09-01 13:33 ` Kumaravel.Thiagarajan
2022-08-30 19:58 ` Geert Uytterhoeven
` (3 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2022-08-30 19:53 UTC (permalink / raw)
To: Kumaravel Thiagarajan
Cc: Greg Kroah-Hartman, Jiri Slaby, Ilpo Järvinen,
Uwe Kleine-König, Johan Hovold, Wander Lairson Costa,
Eric Tremblay, Maciej W. Rozycki, Geert Uytterhoeven,
Jeremy Kerr, Phil Edworthy, Lukas Wunner,
Linux Kernel Mailing List, open list:SERIAL DRIVERS,
Microchip Linux Driver Support
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver.
2022-08-30 18:00 ` [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver Kumaravel Thiagarajan
@ 2022-08-30 19:56 ` Andy Shevchenko
2022-09-01 13:49 ` Kumaravel.Thiagarajan
2022-08-30 23:00 ` kernel test robot
2022-08-31 9:53 ` Ilpo Järvinen
2 siblings, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2022-08-30 19:56 UTC (permalink / raw)
To: Kumaravel Thiagarajan
Cc: Greg Kroah-Hartman, Jiri Slaby, Ilpo Järvinen,
Uwe Kleine-König, Johan Hovold, Wander Lairson Costa,
Eric Tremblay, Maciej W. Rozycki, Geert Uytterhoeven,
Jeremy Kerr, Phil Edworthy, Lukas Wunner,
Linux Kernel Mailing List, open list:SERIAL DRIVERS,
Microchip Linux Driver Support
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* 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.
2022-08-30 18:00 ` [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
2022-08-30 19:53 ` Andy Shevchenko
@ 2022-08-30 19:58 ` Geert Uytterhoeven
2022-09-01 14:09 ` Kumaravel.Thiagarajan
2022-08-30 22:18 ` kernel test robot
` (2 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2022-08-30 19:58 UTC (permalink / raw)
To: Kumaravel Thiagarajan
Cc: Greg KH, Jiri Slaby, Ilpo Jarvinen, Andy Shevchenko,
Uwe Kleine-König, Johan Hovold, wander, etremblay,
Maciej W. Rozycki, Jeremy Kerr, Phil Edworthy, Lukas Wunner,
Linux Kernel Mailing List, open list:SERIAL DRIVERS,
Microchip Linux Driver Support
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* 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.
2022-08-30 18:00 ` [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
2022-08-30 19:53 ` Andy Shevchenko
2022-08-30 19:58 ` Geert Uytterhoeven
@ 2022-08-30 22:18 ` kernel test robot
2022-08-30 22:29 ` kernel test robot
2022-08-31 9:42 ` Ilpo Järvinen
4 siblings, 0 replies; 21+ messages in thread
From: kernel test robot @ 2022-08-30 22:18 UTC (permalink / raw)
To: Kumaravel Thiagarajan, gregkh, jirislaby, ilpo.jarvinen,
andy.shevchenko, u.kleine-koenig, johan, wander, etremblay,
macro, geert+renesas, jk, phil.edworthy, lukas
Cc: kbuild-all, linux-kernel, linux-serial, UNGLinuxDriver
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* 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.
2022-08-30 18:00 ` [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
` (2 preceding siblings ...)
2022-08-30 22:18 ` kernel test robot
@ 2022-08-30 22:29 ` kernel test robot
2022-08-31 9:42 ` Ilpo Järvinen
4 siblings, 0 replies; 21+ messages in thread
From: kernel test robot @ 2022-08-30 22:29 UTC (permalink / raw)
To: Kumaravel Thiagarajan, gregkh, jirislaby, ilpo.jarvinen,
andy.shevchenko, u.kleine-koenig, johan, wander, etremblay,
macro, geert+renesas, jk, phil.edworthy, lukas
Cc: kbuild-all, linux-kernel, linux-serial, UNGLinuxDriver
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver.
2022-08-30 18:00 ` [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver Kumaravel Thiagarajan
2022-08-30 19:56 ` Andy Shevchenko
@ 2022-08-30 23:00 ` kernel test robot
2022-08-31 9:53 ` Ilpo Järvinen
2 siblings, 0 replies; 21+ messages in thread
From: kernel test robot @ 2022-08-30 23:00 UTC (permalink / raw)
To: Kumaravel Thiagarajan, gregkh, jirislaby, ilpo.jarvinen,
andy.shevchenko, u.kleine-koenig, johan, wander, etremblay,
macro, geert+renesas, jk, phil.edworthy, lukas
Cc: kbuild-all, linux-kernel, linux-serial, UNGLinuxDriver
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* 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.
2022-08-30 18:00 ` [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
` (3 preceding siblings ...)
2022-08-30 22:29 ` kernel test robot
@ 2022-08-31 9:42 ` Ilpo Järvinen
2022-09-01 14:21 ` Kumaravel.Thiagarajan
4 siblings, 1 reply; 21+ messages in thread
From: Ilpo Järvinen @ 2022-08-31 9:42 UTC (permalink / raw)
To: Kumaravel Thiagarajan
Cc: Greg Kroah-Hartman, Jiri Slaby, andy.shevchenko, u.kleine-koenig,
johan, wander, etremblay, macro, geert+renesas, jk,
phil.edworthy, Lukas Wunner, LKML, linux-serial, UNGLinuxDriver
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver.
2022-08-30 18:00 ` [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver Kumaravel Thiagarajan
2022-08-30 19:56 ` Andy Shevchenko
2022-08-30 23:00 ` kernel test robot
@ 2022-08-31 9:53 ` Ilpo Järvinen
2022-09-02 2:20 ` Kumaravel.Thiagarajan
2 siblings, 1 reply; 21+ messages in thread
From: Ilpo Järvinen @ 2022-08-31 9:53 UTC (permalink / raw)
To: Kumaravel Thiagarajan
Cc: Greg Kroah-Hartman, Jiri Slaby, andy.shevchenko, u.kleine-koenig,
johan, wander, etremblay, macro, geert+renesas, jk,
phil.edworthy, Lukas Wunner, LKML, linux-serial, UNGLinuxDriver
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* 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.
2022-08-30 19:53 ` Andy Shevchenko
@ 2022-09-01 13:33 ` Kumaravel.Thiagarajan
2022-09-01 13:41 ` Ilpo Järvinen
0 siblings, 1 reply; 21+ messages in thread
From: Kumaravel.Thiagarajan @ 2022-09-01 13:33 UTC (permalink / raw)
To: andy.shevchenko
Cc: gregkh, jirislaby, ilpo.jarvinen, u.kleine-koenig, johan, wander,
etremblay, macro, geert+renesas, jk, phil.edworthy, lukas,
linux-kernel, linux-serial, UNGLinuxDriver
> -----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
^ permalink raw reply [flat|nested] 21+ messages in thread
* 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.
2022-09-01 13:33 ` Kumaravel.Thiagarajan
@ 2022-09-01 13:41 ` Ilpo Järvinen
2022-09-02 11:57 ` Kumaravel.Thiagarajan
0 siblings, 1 reply; 21+ messages in thread
From: Ilpo Järvinen @ 2022-09-01 13:41 UTC (permalink / raw)
To: Kumaravel.Thiagarajan
Cc: andy.shevchenko, Greg Kroah-Hartman, Jiri Slaby, u.kleine-koenig,
johan, wander, etremblay, macro, geert+renesas, jk,
phil.edworthy, Lukas Wunner, LKML, linux-serial, UNGLinuxDriver
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver.
2022-08-30 19:56 ` Andy Shevchenko
@ 2022-09-01 13:49 ` Kumaravel.Thiagarajan
2022-09-29 9:34 ` Kumaravel.Thiagarajan
0 siblings, 1 reply; 21+ messages in thread
From: Kumaravel.Thiagarajan @ 2022-09-01 13:49 UTC (permalink / raw)
To: andy.shevchenko
Cc: gregkh, jirislaby, ilpo.jarvinen, u.kleine-koenig, johan, wander,
etremblay, macro, geert+renesas, jk, phil.edworthy, lukas,
linux-kernel, linux-serial, UNGLinuxDriver
> -----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
^ permalink raw reply [flat|nested] 21+ messages in thread
* 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.
2022-08-30 19:58 ` Geert Uytterhoeven
@ 2022-09-01 14:09 ` Kumaravel.Thiagarajan
0 siblings, 0 replies; 21+ messages in thread
From: Kumaravel.Thiagarajan @ 2022-09-01 14:09 UTC (permalink / raw)
To: geert
Cc: gregkh, jirislaby, ilpo.jarvinen, andy.shevchenko,
u.kleine-koenig, johan, wander, etremblay, macro, jk,
phil.edworthy, lukas, linux-kernel, linux-serial, UNGLinuxDriver
> -----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
^ permalink raw reply [flat|nested] 21+ messages in thread
* 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.
2022-08-31 9:42 ` Ilpo Järvinen
@ 2022-09-01 14:21 ` Kumaravel.Thiagarajan
0 siblings, 0 replies; 21+ messages in thread
From: Kumaravel.Thiagarajan @ 2022-09-01 14:21 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: gregkh, jirislaby, andy.shevchenko, u.kleine-koenig, johan,
wander, etremblay, macro, geert+renesas, jk, phil.edworthy,
lukas, linux-kernel, linux-serial, UNGLinuxDriver
> -----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
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver.
2022-08-31 9:53 ` Ilpo Järvinen
@ 2022-09-02 2:20 ` Kumaravel.Thiagarajan
0 siblings, 0 replies; 21+ messages in thread
From: Kumaravel.Thiagarajan @ 2022-09-02 2:20 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: gregkh, jirislaby, andy.shevchenko, u.kleine-koenig, johan,
wander, etremblay, macro, geert+renesas, jk, phil.edworthy,
lukas, linux-kernel, linux-serial, UNGLinuxDriver
> -----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
^ permalink raw reply [flat|nested] 21+ messages in thread
* 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.
2022-09-01 13:41 ` Ilpo Järvinen
@ 2022-09-02 11:57 ` Kumaravel.Thiagarajan
2022-09-02 15:02 ` Andy Shevchenko
0 siblings, 1 reply; 21+ messages in thread
From: Kumaravel.Thiagarajan @ 2022-09-02 11:57 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: andy.shevchenko, gregkh, jirislaby, u.kleine-koenig, johan,
wander, etremblay, macro, geert+renesas, jk, phil.edworthy,
lukas, linux-kernel, linux-serial, UNGLinuxDriver
> -----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
^ permalink raw reply [flat|nested] 21+ messages in thread
* 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.
2022-09-02 11:57 ` Kumaravel.Thiagarajan
@ 2022-09-02 15:02 ` Andy Shevchenko
2022-09-05 12:01 ` Kumaravel.Thiagarajan
0 siblings, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2022-09-02 15:02 UTC (permalink / raw)
To: Kumaravel Thiagarajan - I21417
Cc: Ilpo Järvinen, Greg Kroah-Hartman, Jiri Slaby,
Uwe Kleine-König, Johan Hovold, Wander Lairson Costa,
Eric Tremblay, Maciej W. Rozycki, Geert Uytterhoeven,
Jeremy Kerr, Phil Edworthy, Lukas Wunner,
Linux Kernel Mailing List, open list:SERIAL DRIVERS,
Microchip Linux Driver Support
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* 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.
2022-09-02 15:02 ` Andy Shevchenko
@ 2022-09-05 12:01 ` Kumaravel.Thiagarajan
0 siblings, 0 replies; 21+ messages in thread
From: Kumaravel.Thiagarajan @ 2022-09-05 12:01 UTC (permalink / raw)
To: andy.shevchenko
Cc: ilpo.jarvinen, gregkh, jirislaby, u.kleine-koenig, johan, wander,
etremblay, macro, geert+renesas, jk, phil.edworthy, lukas,
linux-kernel, linux-serial, UNGLinuxDriver
> -----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
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver.
2022-09-01 13:49 ` Kumaravel.Thiagarajan
@ 2022-09-29 9:34 ` Kumaravel.Thiagarajan
0 siblings, 0 replies; 21+ messages in thread
From: Kumaravel.Thiagarajan @ 2022-09-29 9:34 UTC (permalink / raw)
To: andy.shevchenko
Cc: gregkh, jirislaby, ilpo.jarvinen, u.kleine-koenig, johan, wander,
etremblay, macro, geert+renesas, jk, phil.edworthy, lukas,
linux-kernel, linux-serial, UNGLinuxDriver
> -----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
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2022-09-29 9:35 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-30 18:00 [PATCH v1 tty-next 0/2] 8250: microchip: pci1xxxx: Add driver for the pci1xxxx's quad-uart function Kumaravel Thiagarajan
2022-08-30 18:00 ` [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
2022-08-30 19:53 ` Andy Shevchenko
2022-09-01 13:33 ` Kumaravel.Thiagarajan
2022-09-01 13:41 ` Ilpo Järvinen
2022-09-02 11:57 ` Kumaravel.Thiagarajan
2022-09-02 15:02 ` Andy Shevchenko
2022-09-05 12:01 ` Kumaravel.Thiagarajan
2022-08-30 19:58 ` Geert Uytterhoeven
2022-09-01 14:09 ` Kumaravel.Thiagarajan
2022-08-30 22:18 ` kernel test robot
2022-08-30 22:29 ` kernel test robot
2022-08-31 9:42 ` Ilpo Järvinen
2022-09-01 14:21 ` Kumaravel.Thiagarajan
2022-08-30 18:00 ` [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power management functions to pci1xxxx's quad-uart driver Kumaravel Thiagarajan
2022-08-30 19:56 ` Andy Shevchenko
2022-09-01 13:49 ` Kumaravel.Thiagarajan
2022-09-29 9:34 ` Kumaravel.Thiagarajan
2022-08-30 23:00 ` kernel test robot
2022-08-31 9:53 ` Ilpo Järvinen
2022-09-02 2:20 ` Kumaravel.Thiagarajan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.