| Message ID | 20220830180054.1998296-2-kumaravel.thiagarajan@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | 8250: microchip: pci1xxxx: Add driver for the pci1xxxx's quad-uart function. | expand |
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.
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
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
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
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.
> -----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
> -----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
> -----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
> -----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
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.
> -----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
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 */
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