Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.
From: Andy Shevchenko
Date: Mon Oct 03 2022 - 05:23:44 EST
On Sat, Oct 1, 2022 at 9:15 AM Kumaravel Thiagarajan
<kumaravel.thiagarajan@xxxxxxxxxxxxx> wrote:
>
> pci1xxxx is a PCIe switch with a multi-function endpoint on one of its
> downstream ports. Quad-uart is one of the functions in the
> multi-function endpoint. This driver loads for the quad-uart and
> enumerates single or multiple instances of uart based on the PCIe
> subsystem device ID.
Thanks for an update, my comments below.
...
> +MICROCHIP PCIe UART DRIVER
> +M: Kumaravel Thiagarajan <kumaravel.thiagarajan@xxxxxxxxxxxxx>
> +L: linux-serial@xxxxxxxxxxxxxxx
> +S: Maintained
> +F: drivers/tty/serial/8250/8250_pci1xxxx.c
> 8390 NETWORK DRIVERS [WD80x3/SMC-ELITE, SMC-ULTRA, NE2000, 3C503, etc.]
This doesn't look ordered at all. Please, locate your section in the
appropriate place.
...
> +#include <linux/serial_core.h>
> +#include <linux/8250_pci.h>
> +#include <asm/byteorder.h>
> +#include <linux/bitops.h>
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/pci.h>
> +#include <linux/tty.h>
> +#include <linux/io.h>
Please, keep this ordered and put a blank line between linux/* and
asm/* and "(local)" inclusions.
> +#include "8250.h"
...
> +#define PCI_VENDOR_ID_MCHP_PCI1XXXX 0x1055
Vendor should be vendor, this macro doesn't look right. See pci_ids.h
on how to properly define it.
Btw, PCI_VENDOR_ID_EFAR 0x1055 is there. Use it. (I believe microchip
bought that company in the past?)
...
> +#define PCI_DEVICE_ID_MCHP_PCI12000 0xA002
> +#define PCI_DEVICE_ID_MCHP_PCI11010 0xA012
> +#define PCI_DEVICE_ID_MCHP_PCI11101 0xA022
> +#define PCI_DEVICE_ID_MCHP_PCI11400 0xA032
> +#define PCI_DEVICE_ID_MCHP_PCI11414 0xA042
Use PCI_DEVICE_ID_#vendor_#device format. In a similar way for subdevice IDs.
...
> +#define UART_ACTV_REG 0x11
> +#define UART_ACTV_SET_ACTIVE BIT(0)
This namespace...
> +#define CLK_SEL_REG 0x50
> +#define CLK_SEL_MASK GENMASK(1, 0)
> +#define CLK_SEL_166MHZ 0x01
> +#define CLK_SEL_500MHZ 0x02
> +#define CLK_DIVISOR_REG 0x54
...and this one are potentially conflicting with more generic ones.
Ditto for the rest of the similar definitions.
...
> +static int pci1xxxx_setup_port(struct pci1xxxx_8250 *priv, struct uart_8250_port *port,
> + int offset)
> +{
> + struct pci_dev *dev = priv->dev;
> +
> + if (pci_resource_flags(dev, 0) & IORESOURCE_MEM) {
> + if (!pcim_iomap(dev, 0, 0) && !pcim_iomap_table(dev))
> + return -ENOMEM;
> +
> + port->port.iotype = UPIO_MEM;
> + port->port.iobase = 0;
> + port->port.mapbase = pci_resource_start(dev, 0) + offset;
> + port->port.membase = pcim_iomap_table(dev)[0] + offset;
> + port->port.regshift = 0;
> + } else {
> + port->port.iotype = UPIO_PORT;
> + port->port.iobase = pci_resource_start(dev, 0) + offset;
> + port->port.mapbase = 0;
> + port->port.membase = NULL;
> + port->port.regshift = 0;
> + }
> +
> + return 0;
> +}
Do you really have cards that are providing IO ports? If not, simplify
this accordingly.
...
> + /*
> + * Calculate baud rate sampling period in nano seconds.
nanoseconds
> + * Fractional part x denotes x/255 parts of a nano second.
> + */
...
> + quot = ((1000000000) / (baud * UART_BIT_SAMPLE_CNT));
> + *frac = (((1000000000 - (quot * baud * UART_BIT_SAMPLE_CNT)) /
> + UART_BIT_SAMPLE_CNT) * 255) / baud;
NSEC_PER_SEC ?
...
> + writel((quot << 8) | frac, (port->membase + CLK_DIVISOR_REG));
Too many parentheses.
...
> +static int pci1xxxx_get_num_ports(struct pci_dev *dev)
> +{
> + int nr_ports = 1;
Make this default case.
> +
> + switch (dev->subsystem_device) {
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p0:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p1:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p2:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_1p3:
> + nr_ports = 1;
> + break;
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p01:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p02:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p03:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p12:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p13:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_2p23:
> + nr_ports = 2;
> + break;
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p012:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p123:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p013:
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_3p023:
> + nr_ports = 3;
> + break;
> + case PCI_SUBDEVICE_ID_MCHP_PCI1XXXX_4p:
> + case PCI_SUBDEVICE_ID_MCHP_PCI11414:
> + nr_ports = 4;
> + break;
> + }
> + return nr_ports;
Drop the unnecessary local variable and use return directly from the
switch-cases.
> +}
...
> + int first_offset = 0;
Use default switch-case.
...
> + offset = first_offset + (idx * 256);
Too many parentheses. Check all your code for this kind of unnecessary.
...
> + switch (priv->dev->subsystem_device) {
> + case PCI_SUBDEVICE_ID_MCHP_PCI11414:
> + uart->port.irq = pci_irq_vector(priv->dev, idx);
> + break;
default?
> + }
...
> + uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_TYPE | UPF_FIXED_PORT;
Isn't there something duplicating here what's done in ->setup()?
...
> + uart.port.uartclk = 62500000;
HZ_PER_MHZ ?
...
> + if (num_vectors == 4)
This check should take care of all possible MSI >= 2, correct?
> + writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI, (priv->membase + UART_PCI_CTRL_REG));
> + else
> + uart.port.irq = pci_irq_vector(dev, 0);
...
> + for (i = 0; i < nr_ports; i++) {
> + if (num_vectors == 4)
Ditto.
> + pci1xxxx_irq_assign(priv, &uart, i);
> + priv->line[i] = -ENOSPC;
> + rc = pci1xxxx_setup(priv, &uart, i);
> + if (rc) {
> + dev_err(&dev->dev, "Failed to setup port %u\n", i);
> + break;
> + }
> + priv->line[i] = serial8250_register_8250_port(&uart);
> +
> + if (priv->line[i] < 0) {
> + dev_err(&dev->dev,
> + "Couldn't register serial port %lx, irq %d, type %d, error %d\n",
> + uart.port.iobase, uart.port.irq,
> + uart.port.iotype, priv->line[i]);
> + break;
> + }
> + }
...
> + [PORT_MCHP16550A] = {
> + .name = "MCHP16550A",
> + .fifo_size = 256,
> + .tx_loadsz = 256,
> + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01,
> + .rxtrig_bytes = {2, 66, 130, 194},
> + .flags = UART_CAP_FIFO,
> + },
Can you assign this in ->setup() or so instead of adding a new port type?
> };
...
> +config SERIAL_8250_PCI1XXXX
> + tristate "Microchip 8250 based serial port"
> + depends on SERIAL_8250_PCI
> + default SERIAL_8250
> + help
> + Select this option if you have a setup with Microchip PCIe
> + Switch with serial port enabled and wish to enable 8250
> + serial driver for the serial interface. This driver support
> + will ensure to support baud rates upto 1.5Mpbs.
Keep it in the Quad UART group of config sections (Yes, I know that
not all of them are sorted there, but try your best).
...
> @@ -40,6 +40,7 @@ obj-$(CONFIG_SERIAL_8250_LPSS) += 8250_lpss.o
> obj-$(CONFIG_SERIAL_8250_MID) += 8250_mid.o
> obj-$(CONFIG_SERIAL_8250_PERICOM) += 8250_pericom.o
> obj-$(CONFIG_SERIAL_8250_PXA) += 8250_pxa.o
> +obj-$(CONFIG_SERIAL_8250_PCI1XXXX) += 8250_pci1xxxx.o
> obj-$(CONFIG_SERIAL_8250_TEGRA) += 8250_tegra.o
> obj-$(CONFIG_SERIAL_8250_BCM7271) += 8250_bcm7271.o
> obj-$(CONFIG_SERIAL_OF_PLATFORM) += 8250_of.o
Ditto.
--
With Best Regards,
Andy Shevchenko