Re: [PATCH v8 tty-next 2/4] serial: 8250_pci1xxxx: Add driver for quad-uart support
From: Jiri Slaby
Date: Mon Dec 12 2022 - 02:44:54 EST
On 11. 12. 22, 2:47, 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.
Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@xxxxxxxxxxxxx>
Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@xxxxxxxxxxxxx>
Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@xxxxxxxxxxxxx>
...
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
@@ -0,0 +1,337 @@
...
+struct pci1xxxx_8250 {
+ struct pci_dev *pdev;
+ unsigned int nr;
+ void __iomem *membase;
+ int line[];
+};
...
+static int pci1xxxx_get_max_port(int subsys_dev)
Both look like should be unsigned.
+{
+ static int max_port[] = {
const unsigned
+ 1,/* PCI12000 PCI11010 PCI11101 PCI11400 */
+ 4,/* PCI4p */
+ 3,/* PCI3p012 */
+ 4,/* PCI3p013 */
+ 4,/* PCI3p023 */
+ 4,/* PCI3p123 */
+ 2,/* PCI2p01 */
+ 3,/* PCI2p02 */
+ 4,/* PCI2p03 */
+ 3,/* PCI2p12 */
+ 4,/* PCI2p13 */
+ 4,/* PCI2p23 */
+ 1,/* PCI1p0 */
+ 2,/* PCI1p1 */
+ 3,/* PCI1p2 */
+ 4,/* PCI1p3 */
+ };
+
+ if (subsys_dev > PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3)
+ if (subsys_dev != PCI_SUBDEVICE_ID_EFAR_PCI11414)
+ return max_port[0];
+ else
+ return 4;
+ else
No need for this else. And you should use {}.
+ return max_port[subsys_dev];
+
+}
+
+static int pci1xxxx_logical_to_physical_port_translate(int subsys_dev, int port)
+{
+ static int logical_to_physical_port_idx[][MAX_PORTS] = {
const unsigned.
+ {0, 1, 2, 3},/* PCI12000 PCI11010 PCI11101 PCI11400 PCI11414 */
+ {0, 1, 2, 3},/* PCI4p */
+ {0, 1, 2, -1},/* PCI3p012 */
+ {0, 1, 3, -1},/* PCI3p013 */
+ {0, 2, 3, -1},/* PCI3p023 */
+ {1, 2, 3, -1},/* PCI3p123 */
+ {0, 1, -1, -1},/* PCI2p01 */
+ {0, 2, -1, -1},/* PCI2p02 */
+ {0, 3, -1, -1},/* PCI2p03 */
+ {1, 2, -1, -1},/* PCI2p12 */
+ {1, 3, -1, -1},/* PCI2p13 */
+ {2, 3, -1, -1},/* PCI2p23 */
+ {0, -1, -1, -1},/* PCI1p0 */
+ {1, -1, -1, -1},/* PCI1p1 */
+ {2, -1, -1, -1},/* PCI1p2 */
+ {3, -1, -1, -1},/* PCI1p3 */
+ };
+
+ if (subsys_dev > PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3)
+ return logical_to_physical_port_idx[0][port];
+ else
No need for this else.
+ return logical_to_physical_port_idx[subsys_dev][port];
+}
+
+static int pci1xxxx_serial_probe(struct pci_dev *pdev,
+ const struct pci_device_id *id)
+{
+ struct device *dev = &pdev->dev;
+ struct pci1xxxx_8250 *priv;
+ struct uart_8250_port uart;
+ unsigned int nr_ports, i;
+ int max_vec_reqd;
+ int num_vectors;
+ int subsys_dev;
+ int port_idx;
+ int rc;
+
+ rc = pcim_enable_device(pdev);
+ if (rc)
+ return rc;
+
+ nr_ports = pci1xxxx_get_num_ports(pdev);
+
+ priv = devm_kzalloc(dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->membase = pcim_iomap(pdev, 0, 0);
+ if (!priv->membase)
+ return -ENOMEM;
+
+ priv->pdev = pdev;
What do you need pdev in priv for? It looks superfluous after passing it
to pci1xxxx_setup().
+ subsys_dev = priv->pdev->subsystem_device;
+ priv->nr = nr_ports;
+ pci_set_master(pdev);
+ max_vec_reqd = pci1xxxx_get_max_port(subsys_dev);
+ num_vectors = pci_alloc_irq_vectors(pdev, 1, max_vec_reqd,
+ PCI_IRQ_ALL_TYPES);
+ if (num_vectors < 0)
+ return num_vectors;
+
+ memset(&uart, 0, sizeof(uart));
+ uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_PORT;
+ uart.port.uartclk = UART_CLOCK_DEFAULT;
+ uart.port.dev = dev;
+
+ if (num_vectors == max_vec_reqd)
+ writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI,
+ priv->membase + UART_PCI_CTRL_REG);
+
+ for (i = 0; i < nr_ports; i++)
+ priv->line[i] = -ENODEV;
Why is this not a part of the following (same) loop?
+
+ for (i = 0; i < nr_ports; i++) {
+ port_idx = pci1xxxx_logical_to_physical_port_translate(subsys_dev, i);
+
+ if (num_vectors == max_vec_reqd)
+ uart.port.irq = pci_irq_vector(priv->pdev, port_idx);
+ else
+ uart.port.irq = pci_irq_vector(pdev, 0);
+
+ rc = pci1xxxx_setup(priv, &uart, port_idx);
+ if (rc) {
+ dev_warn(dev, "Failed to setup port %u\n", i);
+ continue;
+ }
+
+ priv->line[i] = serial8250_register_8250_port(&uart);
+ if (priv->line[i] < 0) {
+ dev_warn(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]);
+ }
+ }
+
+ pci_set_drvdata(pdev, priv);
+
+ return 0;
+}
+
+static void pci1xxxx_serial_remove(struct pci_dev *dev)
+{
+ struct pci1xxxx_8250 *priv = pci_get_drvdata(dev);
+ int i;
unsigned as priv->nr is.
+
+ for (i = 0; i < priv->nr; i++) {
+ if (priv->line[i] >= 0)
+ serial8250_unregister_port(priv->line[i]);
+ }
+}
+
+static const struct pci_device_id pci1xxxx_pci_tbl[] = {
+ { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11010) },
+ { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11101) },
+ { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11400) },
+ { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11414) },
+ { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI12000) },
+ {}
thanks,
--
js
suse labs