| Message ID | 20230410211152.94332-1-brenda.streiff@ni.com |
|---|---|
| Headers | show |
| Series | serial: Add driver for National Instruments UARTs | expand |
On 10/04/2023 23:11, Brenda Streiff wrote: > Add bindings for the NI 16550 UART. > > Signed-off-by: Brenda Streiff <brenda.streiff@ni.com> > Cc: Gratian Crisan <gratian.crisan@ni.com> > Cc: Jason Smith <jason.smith@ni.com> Thank you for your patch. There is something to discuss/improve. > --- > .../bindings/serial/ni,ni16550.yaml | 51 +++++++++++++++++++ > 1 file changed, 51 insertions(+) > create mode 100644 Documentation/devicetree/bindings/serial/ni,ni16550.yaml > > diff --git a/Documentation/devicetree/bindings/serial/ni,ni16550.yaml b/Documentation/devicetree/bindings/serial/ni,ni16550.yaml > new file mode 100644 > index 000000000000..13928e89f5aa > --- /dev/null > +++ b/Documentation/devicetree/bindings/serial/ni,ni16550.yaml > @@ -0,0 +1,51 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/serial/ni,ni16550.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: NI 16550 asynchronous serial interface (UART) > + > +maintainers: > + - Brenda Streiff <brenda.streiff@ni.com> > + > +allOf: > + - $ref: serial.yaml# > + > +properties: > + compatible: > + const: ni,ni16550 > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + clock-frequency: true I missed it last time - why do you need this property? You do not have any clock input, so which clock's frequency is it? Best regards, Krzysztof
On 4/11/23 00:44, Krzysztof Kozlowski wrote: > On 10/04/2023 23:11, Brenda Streiff wrote: >> + >> + interrupts: >> + maxItems: 1 >> + >> + clock-frequency: true > > I missed it last time - why do you need this property? You do not have > any clock input, so which clock's frequency is it? > This is the clock frequency of the UART; on our x86-based platforms this comes from the LPC clock, on Zynq-7000 it's derived from a clock in the FPGA. This is used to set struct uart_port::uartclk in the serial core, as it is for other UARTs. This clock frequency can vary based on board design (especially on the x86 side, due to different LPC clocks) but for a given design is fixed- frequency. Would you prefer this be documented further? I was following 8250.yaml's lead here with the simple `true`.
On 13/04/2023 00:24, Brenda Streiff wrote: > On 4/11/23 00:44, Krzysztof Kozlowski wrote: >> On 10/04/2023 23:11, Brenda Streiff wrote: >>> + >>> + interrupts: >>> + maxItems: 1 >>> + >>> + clock-frequency: true >> >> I missed it last time - why do you need this property? You do not have >> any clock input, so which clock's frequency is it? >> > > This is the clock frequency of the UART; on our x86-based platforms this > comes from the LPC clock, on Zynq-7000 it's derived from a clock in the > FPGA. This is used to set struct uart_port::uartclk in the serial core, > as it is for other UARTs. > > This clock frequency can vary based on board design (especially on the > x86 side, due to different LPC clocks) but for a given design is fixed- > frequency. So you must have clock input - clocks property. Once you add this, use assigned-clocks to get the rate you want. > > Would you prefer this be documented further? I was following 8250.yaml's > lead here with the simple `true`. I prefer to drop it. It is not correct and a legacy property. Without clock inputs how can you even configure some clock? Best regards, Krzysztof
On 4/13/23 02:39, Krzysztof Kozlowski wrote: > On 13/04/2023 00:24, Brenda Streiff wrote: >> On 4/11/23 00:44, Krzysztof Kozlowski wrote: >>> On 10/04/2023 23:11, Brenda Streiff wrote: >>>> + >>>> + interrupts: >>>> + maxItems: 1 >>>> + >>>> + clock-frequency: true >>> >>> I missed it last time - why do you need this property? You do not have >>> any clock input, so which clock's frequency is it? >>> >> >> This is the clock frequency of the UART; on our x86-based platforms this >> comes from the LPC clock, on Zynq-7000 it's derived from a clock in the >> FPGA. This is used to set struct uart_port::uartclk in the serial core, >> as it is for other UARTs. >> >> This clock frequency can vary based on board design (especially on the >> x86 side, due to different LPC clocks) but for a given design is fixed- >> frequency. > > So you must have clock input - clocks property. Once you add this, use > assigned-clocks to get the rate you want. > >> >> Would you prefer this be documented further? I was following 8250.yaml's >> lead here with the simple `true`. > > I prefer to drop it. It is not correct and a legacy property. Without > clock inputs how can you even configure some clock? Configure in what respect? Software can't change this clock; it's effectively a fixed oscillator. In practice, this would always be pointing at a compatible="fixed-clock" which declares a clock-frequency; this seems like "clock-frequency but more steps". I can add that, but I'm not clear on what value that adds. We also have shipping devices with ACPI tables using "clock-frequency", so independent of support for "clocks" and "assigned-clocks" for devicetree-using systems, I would still have to keep support in the driver for a "clock-frequency" device property for ACPI-using systems. (Is there documentation on a standalone clock-property being a legacy property that I've missed? I don't see anything of the sort in writing-bindings.rst or in dt-schema and I want to make sure that I haven't missed the proper guidance here.)
On 13/04/2023 22:44, Brenda Streiff wrote: > > > On 4/13/23 02:39, Krzysztof Kozlowski wrote: >> On 13/04/2023 00:24, Brenda Streiff wrote: >>> On 4/11/23 00:44, Krzysztof Kozlowski wrote: >>>> On 10/04/2023 23:11, Brenda Streiff wrote: >>>>> + >>>>> + interrupts: >>>>> + maxItems: 1 >>>>> + >>>>> + clock-frequency: true >>>> >>>> I missed it last time - why do you need this property? You do not have >>>> any clock input, so which clock's frequency is it? >>>> >>> >>> This is the clock frequency of the UART; on our x86-based platforms this >>> comes from the LPC clock, on Zynq-7000 it's derived from a clock in the >>> FPGA. This is used to set struct uart_port::uartclk in the serial core, >>> as it is for other UARTs. >>> >>> This clock frequency can vary based on board design (especially on the >>> x86 side, due to different LPC clocks) but for a given design is fixed- >>> frequency. >> >> So you must have clock input - clocks property. Once you add this, use >> assigned-clocks to get the rate you want. >> >>> >>> Would you prefer this be documented further? I was following 8250.yaml's >>> lead here with the simple `true`. >> >> I prefer to drop it. It is not correct and a legacy property. Without >> clock inputs how can you even configure some clock? > > Configure in what respect? Software can't change this clock; it's > effectively a fixed oscillator. So where is the clock located? Physically. > > In practice, this would always be pointing at a compatible="fixed-clock" > which declares a clock-frequency; this seems like "clock-frequency but > more steps". I can add that, but I'm not clear on what value that adds. Aren't we talking about two different things? Based on limited informamtion, I claimed you have clock input. If you have clock input, then you miss clocks property. What value would that add? I don't know... the rules that bindings describe hardware? > > We also have shipping devices with ACPI tables using "clock-frequency", > so independent of support for "clocks" and "assigned-clocks" for > devicetree-using systems, I would still have to keep support in the > driver for a "clock-frequency" device property for ACPI-using systems. I don't care about ACPI and ACPI has nothing to do with Bindings. We do not write bindings for ACPI. What your driver has or has not to do, it's also separate question. I2C camera sensors solved it long time ago. I don't see why this must be special. > > (Is there documentation on a standalone clock-property being a legacy > property that I've missed? > I don't see anything of the sort in > writing-bindings.rst or in dt-schema and I want to make sure that I dtschema: # Legacy clock properties clock-frequency: description: Legacy property ... > haven't missed the proper guidance here.) Best regards, Krzysztof
Changes from v2 -> v3;
- Schema fixes:
- add "clocks" and "clock-names" properties, comment legacy use of
"clock-frequency".
- Driver fixes:
- get clock rate as a clock consumer, if available
Changes from v1 -> v2:
- Schema fixes:
- Fix schema to now pass "make DT_CHECKER_FLAGS=-m dt_binding_check"
- Rename unprefixed "transceiver" to "ni,serial-port-mode" with a
description. Describing this as a "mode" rather than a "transceiver"
seems more semantically correct in comparison to other DTs; other
schemas with a "transceiver" property (isp1301, lpc32xx-udc) have
phandle values, which is not the case here.
- Driver fixes:
- replace C99 integer types with kernel types
- reverse christmas tree
- stuff PMR mode/cap masks into locals to avoid repetition
- wrap ACPI match table in #ifdef CONFIG_ACPI to avoid build error on
non-ACPI platforms
- NI 16550s are only on x86 and Zynq-7000 NI devices, so constrain the
Kconfig "depends" to only present it as an option to users on those
architectures, to prevent it from being needlessly built on platforms
that would not possibly have them.
- document module name
- add check for "ni,serial-port-mode" property; "transceiver" remains
for sake of compatibility with _DSD ACPI objects for NI controller
BIOSes currently in-field (but also DSD-properties-rules.rst says
that _DSD properties don't have to be identical to DT bindings.)
Brenda Streiff (2):
dt-bindings: serial: ni,ni16650: add bindings
serial: 8250: add driver for NI UARTs
.../bindings/serial/ni,ni16550.yaml | 64 +++
MAINTAINERS | 7 +
drivers/tty/serial/8250/8250_ni.c | 467 ++++++++++++++++++
drivers/tty/serial/8250/Kconfig | 13 +
drivers/tty/serial/8250/Makefile | 1 +
5 files changed, 552 insertions(+)
create mode 100644 Documentation/devicetree/bindings/serial/ni,ni16550.yaml
create mode 100644 drivers/tty/serial/8250/8250_ni.c
On Tue, 18 Apr 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 <brenda.streiff@ni.com> > Cc: Gratian Crisan <gratian.crisan@ni.com> > Cc: Jason Smith <jason.smith@ni.com> > --- > MAINTAINERS | 7 + > drivers/tty/serial/8250/8250_ni.c | 468 ++++++++++++++++++++++++++++++ > drivers/tty/serial/8250/Kconfig | 13 + > drivers/tty/serial/8250/Makefile | 1 + > 4 files changed, 489 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 <brenda.streiff@ni.com> > +L: linux-serial@vger.kernel.org > +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 <zonque@gmail.com> > L: alsa-devel@alsa-project.org (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..92e8912c4875 > --- /dev/null > +++ b/drivers/tty/serial/8250/8250_ni.c > @@ -0,0 +1,468 @@ > +// 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 0x03 Ah, checking the comment, this is GENMASK(1, 0). Remember to add the include for it too. > +#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 0x10 > +#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 0x03 GENMASK() > +#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); Just put these on the same line. > + u8 pcr; > + > + /* "rs485" should be given to us non-NULL. */ > + if (WARN_ON(rs485 == NULL)) > + return -EINVAL; Remove this sanity check. > + 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; > + } Does this difference also mean the UART's RS485 could support SER_RS485_RX_DURING_TX? (half-duplex vs full-duplex RS-485) > + > + 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, The driver does not seem to change anything based on the value of SER_RS485_RTS_ON_SEND, was this an oversight or is RTS on/off on/after send not supported? > +}; > + > +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; Remove the extra alignment, it didn't help much anyway as 1 out of 4 had that |. > + > + 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, "baudclk"); > + 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 = 25000000, > + .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, To me these two comments don't seemingly agree, one states 22.222MHz and defines 25M clk, whereas the other states 29.629MHz and defines 29.629M clk. I guess one of them comes from prescaled and the other from postscaled frequency? > +}; > + > +#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 <jaeden.amero@ni.com>"); > +MODULE_AUTHOR("Karthik Manamcheri <karthik.manamcheri@ni.com>"); > +MODULE_DESCRIPTION("NI 16550 Driver"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig > index 5313aa31930f..c650566fc71d 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. Check that the indent is same for all these lines (tab + 2 spaces). > + > + 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 >
On 4/19/23 06:43, Ilpo Järvinen wrote: > >> + 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; >> + } > > Does this difference also mean the UART's RS485 could support > SER_RS485_RX_DURING_TX? (half-duplex vs full-duplex RS-485) Maybe. At least on some devices it appears that it might be possible to support this with a combination of the NI16550_PCR_ECHO_RS485 mode (this is "2-Wire DTR Controlled With Echo" in NI parlance [1]) along with the AUTO_DTR mode set in ACR (which asserts DTR if there's data in the TX FIFO). We've never tried using it in that way (neither in prior versions of this driver nor our driver for this hardware on Windows), so I'm not sure that works that way on all revisions of the hardware, so I'm hesitant to include that in this patchset for now. [1] https://www.ni.com/en-us/support/documentation/supplemental/18/transceiver-modes-on-ni-rs-485-serial-cards.html >> + >> +static const struct serial_rs485 ni16550_rs485_supported = { >> + .flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RTS_AFTER_SEND, > > The driver does not seem to change anything based on the value of > SER_RS485_RTS_ON_SEND, was this an oversight or is RTS on/off on/after > send not supported? Not so much "not supported" as "doesn't matter"? SER_RS485_RTS_(ON/AFTER)_SEND (which would be a misnomer anyway, as we use DTR and not RTS) appear to let userspace indicate whether or not the transmitter is active-high or active-low, under the assumption that the UART driver doesn't know anything itself about its attached transceiver. This driver does, so it doesn't need either flag to do the right thing. However, there is existing userspace software does try to configure the SER_RS485_RTS_(ON/AFTER)_SEND flags; for instance, pyserial's RS485Settings sets rts_level_for_tx=True [2] and this maps directly to RTS_ON_SEND. Our own proprietary Linux software also sets these flags. So the behavior we want is to we effectively treat them as don't-cares here. I can add comments on that further though, because I can see how that looks a bit confusing. [2] https://github.com/pyserial/pyserial/blob/31fa4807d73ed4eb9891a88a15817b439c4eea2d/serial/rs485.py#L22 >> + >> +/* 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 = 25000000, >> + .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, > > To me these two comments don't seemingly agree, one states 22.222MHz and > defines 25M clk, whereas the other states 29.629MHz and defines 29.629M > clk. I guess one of them comes from prescaled and the other from > postscaled frequency? This was an error-- the comment is correct, I had the uartclk value wrong (should be 22222222).
This patch series adds a driver for the 16550-like UARTs on National Instruments (NI) embedded controller hardware. These UARTs have an interface that is compatble with the TL16C550C (for which we build on top of 8250_core) but also has extra registers for the embedded RS-232/RS-485 tranceiver control circuitry. Changes from v1 -> v2: - Schema fixes: - Fix schema to now pass "make DT_CHECKER_FLAGS=-m dt_binding_check" - Rename unprefixed "transceiver" to "ni,serial-port-mode" with a description. Describing this as a "mode" rather than a "transceiver" seems more semantically correct in comparison to other DTs; other schemas with a "transceiver" property (isp1301, lpc32xx-udc) have phandle values, which is not the case here. - Driver fixes: - replace C99 integer types with kernel types - reverse christmas tree - stuff PMR mode/cap masks into locals to avoid repetition - wrap ACPI match table in #ifdef CONFIG_ACPI to avoid build error on non-ACPI platforms - NI 16550s are only on x86 and Zynq-7000 NI devices, so constrain the Kconfig "depends" to only present it as an option to users on those architectures, to prevent it from being needlessly built on platforms that would not possibly have them. - document module name - add check for "ni,serial-port-mode" property; "transceiver" remains for sake of compatibility with _DSD ACPI objects for NI controller BIOSes currently in-field (but also DSD-properties-rules.rst says that _DSD properties don't have to be identical to DT bindings.) Brenda Streiff (2): dt-bindings: serial: ni,ni16650: add bindings serial: 8250: add driver for NI UARTs .../bindings/serial/ni,ni16550.yaml | 51 ++ MAINTAINERS | 7 + drivers/tty/serial/8250/8250_ni.c | 454 ++++++++++++++++++ drivers/tty/serial/8250/Kconfig | 13 + drivers/tty/serial/8250/Makefile | 1 + 5 files changed, 526 insertions(+) create mode 100644 Documentation/devicetree/bindings/serial/ni,ni16550.yaml create mode 100644 drivers/tty/serial/8250/8250_ni.c