2023-05-05 21:59:10

by Brenda Streiff

[permalink] [raw]
Subject: [PATCH v4 tty-next 2/2] serial: 8250: add driver for NI UARTs

The National Instruments (NI) 16550 is a 16550-like UART with larger
FIFOs and embedded RS-232/RS-485 transceiver control circuitry. This
patch adds a driver that can operate this UART, which is used for
onboard serial ports in several NI embedded controller designs.

Portions of this driver were originally written by Jaeden Amero and
Karthik Manamcheri, with extensive cleanups and refactors since.

Signed-off-by: Brenda Streiff <[email protected]>
Cc: Gratian Crisan <[email protected]>
Cc: Jason Smith <[email protected]>
---
MAINTAINERS | 7 +
drivers/tty/serial/8250/8250_ni.c | 470 ++++++++++++++++++++++++++++++
drivers/tty/serial/8250/Kconfig | 13 +
drivers/tty/serial/8250/Makefile | 1 +
4 files changed, 491 insertions(+)
create mode 100644 drivers/tty/serial/8250/8250_ni.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 90abe83c02f3..4d44622da6cb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14323,6 +14323,13 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next
F: drivers/mtd/nand/
F: include/linux/mtd/*nand*.h

+NATIONAL INSTRUMENTS SERIAL DRIVER
+M: Brenda Streiff <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/serial/ni,ni16550.yaml
+F: drivers/tty/serial/8250/8250_ni.c
+
NATIVE INSTRUMENTS USB SOUND INTERFACE DRIVER
M: Daniel Mack <[email protected]>
L: [email protected] (moderated for non-subscribers)
diff --git a/drivers/tty/serial/8250/8250_ni.c b/drivers/tty/serial/8250/8250_ni.c
new file mode 100644
index 000000000000..b33861d5a431
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_ni.c
@@ -0,0 +1,470 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * NI 16550 UART Driver
+ *
+ * The National Instruments (NI) 16550 is a UART that is compatible with the
+ * TL16C550C and OX16C950B register interfaces, but has additional functions
+ * for RS-485 transceiver control. This driver implements support for the
+ * additional functionality on top of the standard serial8250 core.
+ *
+ * Copyright 2012-2023 National Instruments Corporation
+ */
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/property.h>
+#include <linux/clk.h>
+
+#include "8250.h"
+
+/* Extra bits in UART_ACR */
+#define NI16550_ACR_AUTO_DTR_EN BIT(4)
+
+/* TFS - TX FIFO Size */
+#define NI16550_TFS_OFFSET 0x0C
+/* RFS - RX FIFO Size */
+#define NI16550_RFS_OFFSET 0x0D
+
+/* PMR - Port Mode Register */
+#define NI16550_PMR_OFFSET 0x0E
+/* PMR[1:0] - Port Capabilities */
+#define NI16550_PMR_CAP_MASK GENMASK(1, 0)
+#define NI16550_PMR_NOT_IMPL 0x00 /* not implemented */
+#define NI16550_PMR_CAP_RS232 0x01 /* RS-232 capable */
+#define NI16550_PMR_CAP_RS485 0x02 /* RS-485 capable */
+#define NI16550_PMR_CAP_DUAL 0x03 /* dual-port */
+/* PMR[4] - Interface Mode */
+#define NI16550_PMR_MODE_MASK GENMASK(4, 4)
+#define NI16550_PMR_MODE_RS232 0x00 /* currently 232 */
+#define NI16550_PMR_MODE_RS485 0x10 /* currently 485 */
+
+/* PCR - Port Control Register */
+#define NI16550_PCR_OFFSET 0x0F
+#define NI16550_PCR_RS422 0x00
+#define NI16550_PCR_ECHO_RS485 0x01
+#define NI16550_PCR_DTR_RS485 0x02
+#define NI16550_PCR_AUTO_RS485 0x03
+#define NI16550_PCR_WIRE_MODE_MASK GENMASK(1, 0)
+#define NI16550_PCR_TXVR_ENABLE_BIT BIT(3)
+#define NI16550_PCR_RS485_TERMINATION_BIT BIT(6)
+
+/* flags for ni16550_device_info */
+#define NI_HAS_PMR BIT(0)
+
+struct ni16550_device_info {
+ u32 uartclk;
+ u8 prescaler;
+ u8 flags;
+};
+
+struct ni16550_data {
+ int line;
+ struct clk *clk;
+};
+
+static int ni16550_enable_transceivers(struct uart_port *port)
+{
+ u8 pcr;
+
+ pcr = port->serial_in(port, NI16550_PCR_OFFSET);
+ pcr |= NI16550_PCR_TXVR_ENABLE_BIT;
+ dev_dbg(port->dev, "enable transceivers: write pcr: 0x%02x\n", pcr);
+ port->serial_out(port, NI16550_PCR_OFFSET, pcr);
+
+ return 0;
+}
+
+static int ni16550_disable_transceivers(struct uart_port *port)
+{
+ u8 pcr;
+
+ pcr = port->serial_in(port, NI16550_PCR_OFFSET);
+ pcr &= ~NI16550_PCR_TXVR_ENABLE_BIT;
+ dev_dbg(port->dev, "disable transceivers: write pcr: 0x%02x\n", pcr);
+ port->serial_out(port, NI16550_PCR_OFFSET, pcr);
+
+ return 0;
+}
+
+static int ni16550_rs485_config(struct uart_port *port,
+ struct ktermios *termios,
+ struct serial_rs485 *rs485)
+{
+ struct uart_8250_port *up = container_of(port, struct uart_8250_port, port);
+ u8 pcr;
+
+ pcr = serial_in(up, NI16550_PCR_OFFSET);
+ pcr &= ~NI16550_PCR_WIRE_MODE_MASK;
+
+ if (rs485->flags & SER_RS485_ENABLED) {
+ /* RS-485 */
+ dev_dbg(port->dev, "2-wire Auto\n");
+ pcr |= NI16550_PCR_AUTO_RS485;
+ up->acr |= NI16550_ACR_AUTO_DTR_EN;
+ } else {
+ /* RS-422 */
+ dev_dbg(port->dev, "4-wire\n");
+ pcr |= NI16550_PCR_RS422;
+ up->acr &= ~NI16550_ACR_AUTO_DTR_EN;
+ }
+
+ dev_dbg(port->dev, "config rs485: write pcr: 0x%02x, acr: %02x\n", pcr, up->acr);
+ serial_out(up, NI16550_PCR_OFFSET, pcr);
+ serial_icr_write(up, UART_ACR, up->acr);
+
+ return 0;
+}
+
+static bool is_pmr_rs232_mode(struct uart_8250_port *up)
+{
+ u8 pmr = serial_in(up, NI16550_PMR_OFFSET);
+ u8 pmr_mode = pmr & NI16550_PMR_MODE_MASK;
+ u8 pmr_cap = pmr & NI16550_PMR_CAP_MASK;
+
+ /*
+ * If the PMR is not implemented, then by default NI UARTs are
+ * connected to RS-485 transceivers
+ */
+ if (pmr_cap == NI16550_PMR_NOT_IMPL)
+ return false;
+
+ if (pmr_cap == NI16550_PMR_CAP_DUAL)
+ /*
+ * If the port is dual-mode capable, then read the mode bit
+ * to know the current mode
+ */
+ return pmr_mode == NI16550_PMR_MODE_RS232;
+ /*
+ * If it is not dual-mode capable, then decide based on the
+ * capability
+ */
+ return pmr_cap == NI16550_PMR_CAP_RS232;
+}
+
+static void ni16550_config_prescaler(struct uart_8250_port *up,
+ u8 prescaler)
+{
+ /*
+ * Page in the Enhanced Mode Registers
+ * Sets EFR[4] for Enhanced Mode.
+ */
+ u8 lcr_value;
+ u8 efr_value;
+
+ lcr_value = serial_in(up, UART_LCR);
+ serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+
+ efr_value = serial_in(up, UART_EFR);
+ efr_value |= UART_EFR_ECB;
+
+ serial_out(up, UART_EFR, efr_value);
+
+ /* Page out the Enhanced Mode Registers */
+ serial_out(up, UART_LCR, lcr_value);
+
+ /* Set prescaler to CPR register. */
+ serial_out(up, UART_SCR, UART_CPR);
+ serial_out(up, UART_ICR, prescaler);
+}
+
+static const struct serial_rs485 ni16550_rs485_supported = {
+ .flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RTS_AFTER_SEND,
+ /*
+ * delay_rts_* and RX_DURING_TX are not supported.
+ *
+ * RTS_{ON,AFTER}_SEND are supported, but ignored; the transceiver
+ * is connected in only one way and we don't need userspace to tell
+ * us, but want to retain compatibility with applications that do.
+ */
+};
+
+static void ni16550_rs485_setup(struct uart_port *port)
+{
+ port->rs485_config = ni16550_rs485_config;
+ port->rs485_supported = ni16550_rs485_supported;
+ /*
+ * The hardware comes up by default in 2-wire auto mode and we
+ * set the flags to represent that
+ */
+ port->rs485.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND;
+}
+
+static int ni16550_port_startup(struct uart_port *port)
+{
+ int ret;
+
+ ret = serial8250_do_startup(port);
+ if (ret)
+ return ret;
+
+ return ni16550_enable_transceivers(port);
+}
+
+static void ni16550_port_shutdown(struct uart_port *port)
+{
+ ni16550_disable_transceivers(port);
+
+ serial8250_do_shutdown(port);
+}
+
+static int ni16550_get_regs(struct platform_device *pdev,
+ struct uart_port *port)
+{
+ struct resource *regs;
+
+ regs = platform_get_resource(pdev, IORESOURCE_IO, 0);
+ if (regs) {
+ port->iotype = UPIO_PORT;
+ port->iobase = regs->start;
+
+ return 0;
+ }
+
+ regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (regs) {
+ port->iotype = UPIO_MEM;
+ port->mapbase = regs->start;
+ port->mapsize = resource_size(regs);
+ port->flags |= UPF_IOREMAP;
+
+ port->membase = devm_ioremap(&pdev->dev, port->mapbase,
+ port->mapsize);
+ if (!port->membase)
+ return -ENOMEM;
+
+ return 0;
+ }
+
+ dev_err(&pdev->dev, "no registers defined\n");
+ return -EINVAL;
+}
+
+static u8 ni16550_read_fifo_size(struct uart_8250_port *uart, int reg)
+{
+ /*
+ * Very old implementations don't have the TFS or RFS registers
+ * defined, so we may read all-0s or all-1s. For such devices,
+ * assume a FIFO size of 128.
+ */
+ u8 value = serial_in(uart, reg);
+
+ if (value == 0x00 || value == 0xFF)
+ return 128;
+
+ return value;
+}
+
+static void ni16550_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+ mctrl |= UART_MCR_CLKSEL;
+
+ serial8250_do_set_mctrl(port, mctrl);
+}
+
+static int ni16550_probe(struct platform_device *pdev)
+{
+ const struct ni16550_device_info *info;
+ struct device *dev = &pdev->dev;
+ struct uart_8250_port uart = {};
+ struct ni16550_data *data;
+ const char *portmode;
+ unsigned int prescaler;
+ int txfifosz, rxfifosz;
+ int rs232_property;
+ int ret;
+ int irq;
+
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ spin_lock_init(&uart.port.lock);
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ ret = ni16550_get_regs(pdev, &uart.port);
+ if (ret < 0)
+ return ret;
+
+ /* early setup so that serial_in()/serial_out() work */
+ serial8250_set_defaults(&uart);
+
+ info = device_get_match_data(dev);
+
+ uart.port.dev = dev;
+ uart.port.irq = irq;
+ uart.port.irqflags = IRQF_SHARED;
+ uart.port.flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF
+ | UPF_FIXED_PORT | UPF_FIXED_TYPE;
+ uart.port.startup = ni16550_port_startup;
+ uart.port.shutdown = ni16550_port_shutdown;
+
+ /*
+ * Hardware instantiation of FIFO sizes are held in registers.
+ */
+ txfifosz = ni16550_read_fifo_size(&uart, NI16550_TFS_OFFSET);
+ rxfifosz = ni16550_read_fifo_size(&uart, NI16550_RFS_OFFSET);
+
+ dev_dbg(dev, "NI 16550 has TX FIFO size %d, RX FIFO size %d\n",
+ txfifosz, rxfifosz);
+
+ uart.port.type = PORT_16550A;
+ uart.port.fifosize = txfifosz;
+ uart.tx_loadsz = txfifosz;
+ uart.fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10;
+ uart.capabilities = UART_CAP_FIFO | UART_CAP_AFE | UART_CAP_EFR;
+
+ /*
+ * Declaration of the base clock frequency can come from one of:
+ * - static declaration in this driver (for older ACPI IDs)
+ * - a "clock-frquency" ACPI or OF device property
+ * - an associated OF clock definition
+ */
+ if (info->uartclk)
+ uart.port.uartclk = info->uartclk;
+ if (device_property_read_u32(dev, "clock-frequency",
+ &uart.port.uartclk)) {
+ data->clk = devm_clk_get_optional_enabled(dev, NULL);
+ if (data->clk)
+ uart.port.uartclk = clk_get_rate(data->clk);
+ }
+
+ if (!uart.port.uartclk) {
+ dev_err(dev, "unable to determine clock frequency!\n");
+ ret = -ENODEV;
+ goto err;
+ }
+
+ if (info->prescaler)
+ prescaler = info->prescaler;
+ device_property_read_u32(dev, "clock-prescaler", &prescaler);
+
+ if (prescaler != 0) {
+ uart.port.set_mctrl = ni16550_set_mctrl;
+ ni16550_config_prescaler(&uart, (u8)prescaler);
+ }
+
+ /*
+ * The determination of whether or not this is an RS-485 or RS-232 port
+ * can come from a device property (if present), or it can come from
+ * the PMR (if present), and otherwise we're solely an RS-485 port.
+ *
+ * This is a device-specific property, and thus has a vendor-prefixed
+ * "ni,serial-port-mode" form as a devicetree binding. However, there
+ * are old devices in the field using "transceiver" as an ACPI device
+ * property, so we have to check for that as well.
+ */
+ if (!device_property_read_string(dev, "ni,serial-port-mode",
+ &portmode) ||
+ !device_property_read_string(dev, "transceiver", &portmode)) {
+ rs232_property = strncmp(portmode, "RS-232", 6) == 0;
+
+ dev_dbg(dev, "port is in %s mode (via device property)",
+ (rs232_property ? "RS-232" : "RS-485"));
+ } else if (info->flags & NI_HAS_PMR) {
+ rs232_property = is_pmr_rs232_mode(&uart);
+
+ dev_dbg(dev, "port is in %s mode (via PMR)",
+ (rs232_property ? "RS-232" : "RS-485"));
+ } else {
+ rs232_property = 0;
+
+ dev_dbg(dev, "port is fixed as RS-485");
+ }
+
+ if (!rs232_property) {
+ /*
+ * Neither the 'transceiver' property nor the PMR indicate
+ * that this is an RS-232 port, so it must be an RS-485 one.
+ */
+ ni16550_rs485_setup(&uart.port);
+ }
+
+ ret = serial8250_register_8250_port(&uart);
+ if (ret < 0)
+ goto err;
+ data->line = ret;
+
+ platform_set_drvdata(pdev, data);
+ return 0;
+
+err:
+ clk_disable_unprepare(data->clk);
+ return ret;
+}
+
+static int ni16550_remove(struct platform_device *pdev)
+{
+ struct ni16550_data *data = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(data->clk);
+ serial8250_unregister_port(data->line);
+ return 0;
+}
+
+static const struct ni16550_device_info ni16550_default = { };
+
+static const struct of_device_id ni16550_of_match[] = {
+ { .compatible = "ni,ni16550", .data = &ni16550_default },
+ { },
+};
+MODULE_DEVICE_TABLE(of, ni16550_of_match);
+
+/* NI 16550 RS-485 Interface */
+static const struct ni16550_device_info nic7750 = {
+ .uartclk = 33333333,
+};
+
+/* NI CVS-145x RS-485 Interface */
+static const struct ni16550_device_info nic7772 = {
+ .uartclk = 1843200,
+ .flags = NI_HAS_PMR,
+};
+
+/* NI cRIO-904x RS-485 Interface */
+static const struct ni16550_device_info nic792b = {
+ /* Sets UART clock rate to 22.222 MHz with 1.125 prescale */
+ .uartclk = 22222222,
+ .prescaler = 0x09,
+};
+
+/* NI sbRIO 96x8 RS-232/485 Interfaces */
+static const struct ni16550_device_info nic7a69 = {
+ /* Set UART clock rate to 29.629 MHz with 1.125 prescale */
+ .uartclk = 29629629,
+ .prescaler = 0x09,
+};
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id ni16550_acpi_match[] = {
+ { "NIC7750", (kernel_ulong_t)&nic7750 },
+ { "NIC7772", (kernel_ulong_t)&nic7772 },
+ { "NIC792B", (kernel_ulong_t)&nic792b },
+ { "NIC7A69", (kernel_ulong_t)&nic7a69 },
+ { },
+};
+MODULE_DEVICE_TABLE(acpi, ni16550_acpi_match);
+#endif
+
+static struct platform_driver ni16550_driver = {
+ .driver = {
+ .name = "ni16550",
+ .of_match_table = ni16550_of_match,
+ .acpi_match_table = ACPI_PTR(ni16550_acpi_match),
+ },
+ .probe = ni16550_probe,
+ .remove = ni16550_remove,
+};
+
+module_platform_driver(ni16550_driver);
+
+MODULE_AUTHOR("Jaeden Amero <[email protected]>");
+MODULE_AUTHOR("Karthik Manamcheri <[email protected]>");
+MODULE_DESCRIPTION("NI 16550 Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 5313aa31930f..2d9f917e96a0 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -565,6 +565,19 @@ config SERIAL_8250_BCM7271
including DMA support and high accuracy BAUD rates, say
Y to this option. If unsure, say N.

+config SERIAL_8250_NI
+ tristate "NI 16550 based serial port"
+ depends on SERIAL_8250
+ depends on (X86 && ACPI) || (ARCH_ZYNQ && OF) || COMPILE_TEST
+ help
+ This driver supports the integrated serial ports on National
+ Instruments (NI) controller hardware. This is required for all NI
+ controller models with onboard RS-485 or dual-mode RS-485/RS-232
+ ports.
+
+ To compile this driver as a module, choose M here: the module
+ will be called 8250_ni.
+
config SERIAL_OF_PLATFORM
tristate "Devicetree based probing for 8250 ports"
depends on SERIAL_8250 && OF
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 4fc2fc1f41b6..58dc1b5ff054 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_SERIAL_8250_PERICOM) += 8250_pericom.o
obj-$(CONFIG_SERIAL_8250_PXA) += 8250_pxa.o
obj-$(CONFIG_SERIAL_8250_TEGRA) += 8250_tegra.o
obj-$(CONFIG_SERIAL_8250_BCM7271) += 8250_bcm7271.o
+obj-$(CONFIG_SERIAL_8250_NI) += 8250_ni.o
obj-$(CONFIG_SERIAL_OF_PLATFORM) += 8250_of.o

CFLAGS_8250_ingenic.o += -I$(srctree)/scripts/dtc/libfdt
--
2.30.2


2023-05-08 08:24:01

by Ilpo Järvinen

[permalink] [raw]
Subject: Re: [PATCH v4 tty-next 2/2] serial: 8250: add driver for NI UARTs

On Fri, 5 May 2023, Brenda Streiff wrote:

> The National Instruments (NI) 16550 is a 16550-like UART with larger
> FIFOs and embedded RS-232/RS-485 transceiver control circuitry. This
> patch adds a driver that can operate this UART, which is used for
> onboard serial ports in several NI embedded controller designs.
>
> Portions of this driver were originally written by Jaeden Amero and
> Karthik Manamcheri, with extensive cleanups and refactors since.
>
> Signed-off-by: Brenda Streiff <[email protected]>
> Cc: Gratian Crisan <[email protected]>
> Cc: Jason Smith <[email protected]>
> ---
> MAINTAINERS | 7 +
> drivers/tty/serial/8250/8250_ni.c | 470 ++++++++++++++++++++++++++++++
> drivers/tty/serial/8250/Kconfig | 13 +
> drivers/tty/serial/8250/Makefile | 1 +
> 4 files changed, 491 insertions(+)
> create mode 100644 drivers/tty/serial/8250/8250_ni.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90abe83c02f3..4d44622da6cb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14323,6 +14323,13 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next
> F: drivers/mtd/nand/
> F: include/linux/mtd/*nand*.h
>
> +NATIONAL INSTRUMENTS SERIAL DRIVER
> +M: Brenda Streiff <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: Documentation/devicetree/bindings/serial/ni,ni16550.yaml
> +F: drivers/tty/serial/8250/8250_ni.c
> +
> NATIVE INSTRUMENTS USB SOUND INTERFACE DRIVER
> M: Daniel Mack <[email protected]>
> L: [email protected] (moderated for non-subscribers)
> diff --git a/drivers/tty/serial/8250/8250_ni.c b/drivers/tty/serial/8250/8250_ni.c
> new file mode 100644
> index 000000000000..b33861d5a431
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_ni.c
> @@ -0,0 +1,470 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * NI 16550 UART Driver
> + *
> + * The National Instruments (NI) 16550 is a UART that is compatible with the
> + * TL16C550C and OX16C950B register interfaces, but has additional functions
> + * for RS-485 transceiver control. This driver implements support for the
> + * additional functionality on top of the standard serial8250 core.
> + *
> + * Copyright 2012-2023 National Instruments Corporation
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/property.h>
> +#include <linux/clk.h>
> +
> +#include "8250.h"
> +
> +/* Extra bits in UART_ACR */
> +#define NI16550_ACR_AUTO_DTR_EN BIT(4)
> +
> +/* TFS - TX FIFO Size */
> +#define NI16550_TFS_OFFSET 0x0C
> +/* RFS - RX FIFO Size */
> +#define NI16550_RFS_OFFSET 0x0D
> +
> +/* PMR - Port Mode Register */
> +#define NI16550_PMR_OFFSET 0x0E
> +/* PMR[1:0] - Port Capabilities */
> +#define NI16550_PMR_CAP_MASK GENMASK(1, 0)
> +#define NI16550_PMR_NOT_IMPL 0x00 /* not implemented */
> +#define NI16550_PMR_CAP_RS232 0x01 /* RS-232 capable */
> +#define NI16550_PMR_CAP_RS485 0x02 /* RS-485 capable */
> +#define NI16550_PMR_CAP_DUAL 0x03 /* dual-port */
> +/* PMR[4] - Interface Mode */
> +#define NI16550_PMR_MODE_MASK GENMASK(4, 4)
> +#define NI16550_PMR_MODE_RS232 0x00 /* currently 232 */
> +#define NI16550_PMR_MODE_RS485 0x10 /* currently 485 */
> +
> +/* PCR - Port Control Register */
> +#define NI16550_PCR_OFFSET 0x0F
> +#define NI16550_PCR_RS422 0x00
> +#define NI16550_PCR_ECHO_RS485 0x01
> +#define NI16550_PCR_DTR_RS485 0x02
> +#define NI16550_PCR_AUTO_RS485 0x03
> +#define NI16550_PCR_WIRE_MODE_MASK GENMASK(1, 0)
> +#define NI16550_PCR_TXVR_ENABLE_BIT BIT(3)
> +#define NI16550_PCR_RS485_TERMINATION_BIT BIT(6)
> +
> +/* flags for ni16550_device_info */
> +#define NI_HAS_PMR BIT(0)
> +
> +struct ni16550_device_info {
> + u32 uartclk;
> + u8 prescaler;
> + u8 flags;
> +};
> +
> +struct ni16550_data {
> + int line;
> + struct clk *clk;
> +};
> +
> +static int ni16550_enable_transceivers(struct uart_port *port)
> +{
> + u8 pcr;
> +
> + pcr = port->serial_in(port, NI16550_PCR_OFFSET);
> + pcr |= NI16550_PCR_TXVR_ENABLE_BIT;
> + dev_dbg(port->dev, "enable transceivers: write pcr: 0x%02x\n", pcr);
> + port->serial_out(port, NI16550_PCR_OFFSET, pcr);
> +
> + return 0;
> +}
> +
> +static int ni16550_disable_transceivers(struct uart_port *port)
> +{
> + u8 pcr;
> +
> + pcr = port->serial_in(port, NI16550_PCR_OFFSET);
> + pcr &= ~NI16550_PCR_TXVR_ENABLE_BIT;
> + dev_dbg(port->dev, "disable transceivers: write pcr: 0x%02x\n", pcr);
> + port->serial_out(port, NI16550_PCR_OFFSET, pcr);
> +
> + return 0;
> +}
> +
> +static int ni16550_rs485_config(struct uart_port *port,
> + struct ktermios *termios,
> + struct serial_rs485 *rs485)
> +{
> + struct uart_8250_port *up = container_of(port, struct uart_8250_port, port);
> + u8 pcr;
> +
> + pcr = serial_in(up, NI16550_PCR_OFFSET);
> + pcr &= ~NI16550_PCR_WIRE_MODE_MASK;
> +
> + if (rs485->flags & SER_RS485_ENABLED) {
> + /* RS-485 */
> + dev_dbg(port->dev, "2-wire Auto\n");
> + pcr |= NI16550_PCR_AUTO_RS485;
> + up->acr |= NI16550_ACR_AUTO_DTR_EN;
> + } else {
> + /* RS-422 */
> + dev_dbg(port->dev, "4-wire\n");
> + pcr |= NI16550_PCR_RS422;
> + up->acr &= ~NI16550_ACR_AUTO_DTR_EN;
> + }
> +
> + dev_dbg(port->dev, "config rs485: write pcr: 0x%02x, acr: %02x\n", pcr, up->acr);
> + serial_out(up, NI16550_PCR_OFFSET, pcr);
> + serial_icr_write(up, UART_ACR, up->acr);
> +
> + return 0;
> +}
> +
> +static bool is_pmr_rs232_mode(struct uart_8250_port *up)
> +{
> + u8 pmr = serial_in(up, NI16550_PMR_OFFSET);
> + u8 pmr_mode = pmr & NI16550_PMR_MODE_MASK;
> + u8 pmr_cap = pmr & NI16550_PMR_CAP_MASK;
> +
> + /*
> + * If the PMR is not implemented, then by default NI UARTs are
> + * connected to RS-485 transceivers
> + */
> + if (pmr_cap == NI16550_PMR_NOT_IMPL)
> + return false;
> +
> + if (pmr_cap == NI16550_PMR_CAP_DUAL)
> + /*
> + * If the port is dual-mode capable, then read the mode bit
> + * to know the current mode
> + */
> + return pmr_mode == NI16550_PMR_MODE_RS232;
> + /*
> + * If it is not dual-mode capable, then decide based on the
> + * capability
> + */
> + return pmr_cap == NI16550_PMR_CAP_RS232;
> +}
> +
> +static void ni16550_config_prescaler(struct uart_8250_port *up,
> + u8 prescaler)
> +{
> + /*
> + * Page in the Enhanced Mode Registers
> + * Sets EFR[4] for Enhanced Mode.
> + */
> + u8 lcr_value;
> + u8 efr_value;
> +
> + lcr_value = serial_in(up, UART_LCR);
> + serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
> +
> + efr_value = serial_in(up, UART_EFR);
> + efr_value |= UART_EFR_ECB;
> +
> + serial_out(up, UART_EFR, efr_value);
> +
> + /* Page out the Enhanced Mode Registers */
> + serial_out(up, UART_LCR, lcr_value);
> +
> + /* Set prescaler to CPR register. */
> + serial_out(up, UART_SCR, UART_CPR);
> + serial_out(up, UART_ICR, prescaler);
> +}
> +
> +static const struct serial_rs485 ni16550_rs485_supported = {
> + .flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RTS_AFTER_SEND,
> + /*
> + * delay_rts_* and RX_DURING_TX are not supported.
> + *
> + * RTS_{ON,AFTER}_SEND are supported, but ignored; the transceiver
> + * is connected in only one way and we don't need userspace to tell
> + * us, but want to retain compatibility with applications that do.

This is a bit odd statement. If application wants to specify those flags,
they will be ignored (cleared) by serial core without triggering an error
even when driver does not claim to support them.

Or are you saying some application sets these flags and fails if it
doesn't get the same flags back?

> + */
> +};
> +
> +static void ni16550_rs485_setup(struct uart_port *port)
> +{
> + port->rs485_config = ni16550_rs485_config;
> + port->rs485_supported = ni16550_rs485_supported;
> + /*
> + * The hardware comes up by default in 2-wire auto mode and we
> + * set the flags to represent that
> + */
> + port->rs485.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND;
> +}
> +
> +static int ni16550_port_startup(struct uart_port *port)
> +{
> + int ret;
> +
> + ret = serial8250_do_startup(port);
> + if (ret)
> + return ret;
> +
> + return ni16550_enable_transceivers(port);
> +}
> +
> +static void ni16550_port_shutdown(struct uart_port *port)
> +{
> + ni16550_disable_transceivers(port);
> +
> + serial8250_do_shutdown(port);
> +}
> +
> +static int ni16550_get_regs(struct platform_device *pdev,
> + struct uart_port *port)
> +{
> + struct resource *regs;
> +
> + regs = platform_get_resource(pdev, IORESOURCE_IO, 0);
> + if (regs) {
> + port->iotype = UPIO_PORT;
> + port->iobase = regs->start;
> +
> + return 0;
> + }
> +
> + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (regs) {
> + port->iotype = UPIO_MEM;
> + port->mapbase = regs->start;
> + port->mapsize = resource_size(regs);
> + port->flags |= UPF_IOREMAP;
> +
> + port->membase = devm_ioremap(&pdev->dev, port->mapbase,
> + port->mapsize);
> + if (!port->membase)
> + return -ENOMEM;
> +
> + return 0;
> + }
> +
> + dev_err(&pdev->dev, "no registers defined\n");
> + return -EINVAL;
> +}
> +
> +static u8 ni16550_read_fifo_size(struct uart_8250_port *uart, int reg)
> +{
> + /*
> + * Very old implementations don't have the TFS or RFS registers
> + * defined, so we may read all-0s or all-1s. For such devices,
> + * assume a FIFO size of 128.
> + */
> + u8 value = serial_in(uart, reg);
> +
> + if (value == 0x00 || value == 0xFF)
> + return 128;
> +
> + return value;
> +}
> +
> +static void ni16550_set_mctrl(struct uart_port *port, unsigned int mctrl)
> +{
> + mctrl |= UART_MCR_CLKSEL;
> +
> + serial8250_do_set_mctrl(port, mctrl);
> +}
> +
> +static int ni16550_probe(struct platform_device *pdev)
> +{
> + const struct ni16550_device_info *info;
> + struct device *dev = &pdev->dev;
> + struct uart_8250_port uart = {};
> + struct ni16550_data *data;
> + const char *portmode;
> + unsigned int prescaler;
> + int txfifosz, rxfifosz;
> + int rs232_property;
> + int ret;
> + int irq;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + spin_lock_init(&uart.port.lock);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + ret = ni16550_get_regs(pdev, &uart.port);
> + if (ret < 0)
> + return ret;
> +
> + /* early setup so that serial_in()/serial_out() work */
> + serial8250_set_defaults(&uart);
> +
> + info = device_get_match_data(dev);
> +
> + uart.port.dev = dev;
> + uart.port.irq = irq;
> + uart.port.irqflags = IRQF_SHARED;
> + uart.port.flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF
> + | UPF_FIXED_PORT | UPF_FIXED_TYPE;
> + uart.port.startup = ni16550_port_startup;
> + uart.port.shutdown = ni16550_port_shutdown;
> +
> + /*
> + * Hardware instantiation of FIFO sizes are held in registers.
> + */
> + txfifosz = ni16550_read_fifo_size(&uart, NI16550_TFS_OFFSET);
> + rxfifosz = ni16550_read_fifo_size(&uart, NI16550_RFS_OFFSET);
> +
> + dev_dbg(dev, "NI 16550 has TX FIFO size %d, RX FIFO size %d\n",
> + txfifosz, rxfifosz);
> +
> + uart.port.type = PORT_16550A;
> + uart.port.fifosize = txfifosz;
> + uart.tx_loadsz = txfifosz;
> + uart.fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10;
> + uart.capabilities = UART_CAP_FIFO | UART_CAP_AFE | UART_CAP_EFR;
> +
> + /*
> + * Declaration of the base clock frequency can come from one of:
> + * - static declaration in this driver (for older ACPI IDs)
> + * - a "clock-frquency" ACPI or OF device property
> + * - an associated OF clock definition
> + */
> + if (info->uartclk)
> + uart.port.uartclk = info->uartclk;
> + if (device_property_read_u32(dev, "clock-frequency",
> + &uart.port.uartclk)) {
> + data->clk = devm_clk_get_optional_enabled(dev, NULL);
> + if (data->clk)
> + uart.port.uartclk = clk_get_rate(data->clk);
> + }
> +
> + if (!uart.port.uartclk) {
> + dev_err(dev, "unable to determine clock frequency!\n");
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + if (info->prescaler)
> + prescaler = info->prescaler;
> + device_property_read_u32(dev, "clock-prescaler", &prescaler);
> +
> + if (prescaler != 0) {

device_property_read_u32() can fail w/o filling &prescaler so it doesn't
seem to be always initialized properly.

> + uart.port.set_mctrl = ni16550_set_mctrl;
> + ni16550_config_prescaler(&uart, (u8)prescaler);
> + }
> +
> + /*
> + * The determination of whether or not this is an RS-485 or RS-232 port
> + * can come from a device property (if present), or it can come from
> + * the PMR (if present), and otherwise we're solely an RS-485 port.
> + *
> + * This is a device-specific property, and thus has a vendor-prefixed
> + * "ni,serial-port-mode" form as a devicetree binding. However, there
> + * are old devices in the field using "transceiver" as an ACPI device
> + * property, so we have to check for that as well.
> + */
> + if (!device_property_read_string(dev, "ni,serial-port-mode",
> + &portmode) ||

Just put these into the same line.

> + !device_property_read_string(dev, "transceiver", &portmode)) {
> + rs232_property = strncmp(portmode, "RS-232", 6) == 0;
> +
> + dev_dbg(dev, "port is in %s mode (via device property)",
> + (rs232_property ? "RS-232" : "RS-485"));
> + } else if (info->flags & NI_HAS_PMR) {
> + rs232_property = is_pmr_rs232_mode(&uart);
> +
> + dev_dbg(dev, "port is in %s mode (via PMR)",
> + (rs232_property ? "RS-232" : "RS-485"));

Unnecessary parenthesis.

--
i.

> + } else {
> + rs232_property = 0;
> +
> + dev_dbg(dev, "port is fixed as RS-485");
> + }
> +
> + if (!rs232_property) {
> + /*
> + * Neither the 'transceiver' property nor the PMR indicate
> + * that this is an RS-232 port, so it must be an RS-485 one.
> + */
> + ni16550_rs485_setup(&uart.port);
> + }
> +
> + ret = serial8250_register_8250_port(&uart);
> + if (ret < 0)
> + goto err;
> + data->line = ret;
> +
> + platform_set_drvdata(pdev, data);
> + return 0;
> +
> +err:
> + clk_disable_unprepare(data->clk);
> + return ret;
> +}
> +
> +static int ni16550_remove(struct platform_device *pdev)
> +{
> + struct ni16550_data *data = platform_get_drvdata(pdev);
> +
> + clk_disable_unprepare(data->clk);
> + serial8250_unregister_port(data->line);
> + return 0;
> +}
> +
> +static const struct ni16550_device_info ni16550_default = { };
> +
> +static const struct of_device_id ni16550_of_match[] = {
> + { .compatible = "ni,ni16550", .data = &ni16550_default },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, ni16550_of_match);
> +
> +/* NI 16550 RS-485 Interface */
> +static const struct ni16550_device_info nic7750 = {
> + .uartclk = 33333333,
> +};
> +
> +/* NI CVS-145x RS-485 Interface */
> +static const struct ni16550_device_info nic7772 = {
> + .uartclk = 1843200,
> + .flags = NI_HAS_PMR,
> +};
> +
> +/* NI cRIO-904x RS-485 Interface */
> +static const struct ni16550_device_info nic792b = {
> + /* Sets UART clock rate to 22.222 MHz with 1.125 prescale */
> + .uartclk = 22222222,
> + .prescaler = 0x09,
> +};
> +
> +/* NI sbRIO 96x8 RS-232/485 Interfaces */
> +static const struct ni16550_device_info nic7a69 = {
> + /* Set UART clock rate to 29.629 MHz with 1.125 prescale */
> + .uartclk = 29629629,
> + .prescaler = 0x09,
> +};
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id ni16550_acpi_match[] = {
> + { "NIC7750", (kernel_ulong_t)&nic7750 },
> + { "NIC7772", (kernel_ulong_t)&nic7772 },
> + { "NIC792B", (kernel_ulong_t)&nic792b },
> + { "NIC7A69", (kernel_ulong_t)&nic7a69 },
> + { },
> +};
> +MODULE_DEVICE_TABLE(acpi, ni16550_acpi_match);
> +#endif
> +
> +static struct platform_driver ni16550_driver = {
> + .driver = {
> + .name = "ni16550",
> + .of_match_table = ni16550_of_match,
> + .acpi_match_table = ACPI_PTR(ni16550_acpi_match),
> + },
> + .probe = ni16550_probe,
> + .remove = ni16550_remove,
> +};
> +
> +module_platform_driver(ni16550_driver);
> +
> +MODULE_AUTHOR("Jaeden Amero <[email protected]>");
> +MODULE_AUTHOR("Karthik Manamcheri <[email protected]>");
> +MODULE_DESCRIPTION("NI 16550 Driver");
> +MODULE_LICENSE("GPL");

2023-06-23 18:59:34

by Brenda Streiff

[permalink] [raw]
Subject: Re: [PATCH v4 tty-next 2/2] serial: 8250: add driver for NI UARTs

Apologies for the delay in response. I have your other requested
changes in a pending v5 patchset, but answering this question took
some digging (and I got bounced between projects in the intervening
time)

On 5/8/23 03:01, Ilpo Järvinen wrote:
> On Fri, 5 May 2023, Brenda Streiff wrote:

>> +static const struct serial_rs485 ni16550_rs485_supported = {
>> + .flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RTS_AFTER_SEND,
>> + /*
>> + * delay_rts_* and RX_DURING_TX are not supported.
>> + *
>> + * RTS_{ON,AFTER}_SEND are supported, but ignored; the transceiver
>> + * is connected in only one way and we don't need userspace to tell
>> + * us, but want to retain compatibility with applications that do.
>
> This is a bit odd statement. If application wants to specify those flags,
> they will be ignored (cleared) by serial core without triggering an error
> even when driver does not claim to support them.
>
> Or are you saying some application sets these flags and fails if it
> doesn't get the same flags back?

Yes, this is the case.

There's an unfortunate amount of historical baggage on our side here.

The hardware supports four modes:
Wire Mode | Tx enabled? | Rx enabled?
-----------------------|--------------------|------------------------
NI16650_PCR_RS422 | Always | Always
NI16650_PCR_ECHO_RS485 | If DTR asserted | Always
NI16650_PCR_DTR_RS485 | If DTR asserted | Disabled when TX enabled
NI16650_PCR_AUTO_RS485 | If data in TX FIFO | Disabled when TX enabled

These four hardware modes map directly to "wire mode" settings in higher
level NI software APIs, such as NI-VISA's VI_ATTR_ASRL_WIRE_MODE
property [1] on other operating systems, and our drivers for those OSes
just present those settings directly in their ABI. Linux already had
support for RS485, but when the driver was first authored for Linux [2]
(and then kept out-of-tree for a decade), it was done so with an
(erroneous) understanding of the RS485 flags:

Wire Mode struct serial_rs485::flags
----------------------- --------------------
NI16650_PCR_RS422: 0
NI16650_PCR_ECHO_RS485: SER_RS485_ENABLED|SER_RS485_RX_DURING_TX
NI16650_PCR_DTR_RS485: SER_RS485_ENABLED
NI16650_PCR_AUTO_RS485: SER_RS485_ENABLED|SER_RS485_RTS_ON_SEND

The "RS485 serial_rs485 sanitization" patchset [3] ended up breaking us
and forced us to reevaluate it, because now some of those states were
invalid (a driver cannot now simultaneously support not having
RTS_ON_SEND at the same time as supporting having RTS_ON_SEND).

Ultimately we made the decision that we only care about the RS422 and
AUTO_RS485 modes, but now we have a slightly different problem, in that
there's a bunch of userspace middleware that is still coded to expect
all four modes; so if we remove support for the SER_RS485_RTS_ON_SEND
flag, even though we don't need it, we end up with:
- middleware sets SER_RS485_ENABLED|SER_RS485_RTS_ON_SEND
- kernel coerces this into SER_RS485_ENABLED
- middleware reads back the value, sees the flags are different, and
assumes that the mode isn't properly supported and errors.

So we need to "support" SER_RS485_RTS_ON_SEND, even though we don't
require the presence or absence of the flag to direct any hardware
configuration change, in order to avoid this coercion happening.

Ideally we would have a userspace more tolerant of the kernel now
coercing flags differently than it used to, but unfortunately we're
stuck dealing with it for a while.

If there's a better way you'd prefer this be handled, let me know.
We're trying to walk the line between "the driver needs to present
the ABI correctly to userspace" and "our userspace relied on it having
been incorrectly implemented in the past".

[1] https://www.ni.com/docs/en-US/bundle/ni-visa/page/ni-visa/vi_attr_asrl_wire_mode.html
[2] https://github.com/ni/linux/blob/nilrt_pub/14.0/3.10/drivers/tty/serial/8250/8250_ni16550.c#L96
[3] https://lore.kernel.org/all/[email protected]/