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