Skip to content

serial: 8250_ni: new driver for NI UARTs#100

Closed
bstreiff wants to merge 4 commits intoni:nilrt/master/6.0from
bstreiff:dev/bstreiff/6.0/ni16550
Closed

serial: 8250_ni: new driver for NI UARTs#100
bstreiff wants to merge 4 commits intoni:nilrt/master/6.0from
bstreiff:dev/bstreiff/6.0/ni16550

Conversation

@bstreiff
Copy link
Contributor

The National Instruments (NI) 16550 is a standard 16550 with larger FIFOs and embedded RS-232/RS-485 transceiver control circuitry. This patch adds a driver that can operate this UART.


This patch set is a combination and reformulation of

  • 2d4c945 serial_core: Provide a transceiver interface
  • 8e9e459 8250: Allow client drivers to control transceivers
  • a07f249 8250: Add a driver for NI 16550 UART
  • 6fbc61e 8250_pnp: Make NIC7750 ID spawn a NI 16550 port
  • 8d2f96d 8250_pnp: Support dual-mode UART through PMR
  • 053fe96 8250_pnp: NI ACPI IDs start with NIC
  • 7e4f38d 8250: NI 16550 can have different FIFO sizes
  • 806fb0d 8250_pnp: Provide NI_16BYTE_FIFO flag
  • 46312fa 8250_pnp: Make NIC7772 spawn a dual-mode UART
  • 3089f71 8250_pnp: Add new ID NIC792B for NI 16550 UART 25MHz
  • 266b84d serial: 8250_ni16550: Add support for NIC7A69
  • bb3aac8 8250: Use proper set_mctrl callback for ni16550 clock divider
  • d0e236d 8250_ni16550: fix compiler warning about mixed declarations and code
  • 145e892 8250_ni16550: pass termios as param to ni16550_config_rs485

There are significant differences between that pile of patches and what we have here, which is:

  • The operative configuration token is CONFIG_SERIAL_8250_NI instead of CONFIG_SERIAL_8250_NI16550-- this lets the two implementations live side-by-side in our tree (rationale below)
  • The generic "transceiver interface" that was added in the earliest patches is now gone-- transceiver enable/disable just happens in the port startup/shutdown callbacks now. Having that as a separate API might have been nice, but I think it'd only be justifiable if I ported other 8250 drivers to use it, and the things that it did just happened in startup/shutdown anyway so it's not really a "missing" function of the 8250 core.
  • We're now our own platform_device instead of a pile of custom initialization bodged into the 8250_pnp code. This pushes more of the NI16550-specific code into its own file (and reducing future merge conflicts)
  • Because we're now our own platform_device, on x64, device enumeration happens a little bit differently than enumeration via the legacy PNP system. This should be invisible to most users unless they go digging real hard in sysfs ($(realpath /sys/class/tty/ttyS1) on a cRIO-904x becomes /sys/devices/pci0000:00/0000:00:1f.0/NIC792B:00/tty/ttyS1 instead of /sys/devices/pnp0/00:01/tty/ttyS1-- however most users are probably just going to be using /dev/ttyS1, which doesn't change.)
  • PORT_NI16550_* is gone; we just use PORT_16550A. This is a visible change to userspace (/sys/class/ttyS1/type and also via serial_struct::type with the TIOCGSERIAL ioctl), but userspace could not rely on the values we had been using anyway (since we had to keep renumbering on every kernel upgrade). Note that use of this field for identification purposes seems to have fallen out of fashion-- in fact for USB serial devices as of f64d74a they all just return PORT_UNKNOWN.
  • NI_16BYTE_FIFO is gone now (as are the distinct ni,ni16550-f16 and ni,ni16550-f128 OF names)-- instead we look at the TFS/RFS registers; these have been present in the common UART core since at least 2012, and if they're somehow not there, then 128 bytes seems fine as an assumption, since the single 16-byte-FIFO instantiation seems to have been the exception.
  • NI_CAP_PMR has a slightly more interesting tale-- it was added to the common core in 2014, shipped in the CVS product line, and then removed with no explanation in 2015. (Filed AzDO 2275198 to HW about that.) At any rate, in the current version of the register map, that register is now no longer reserved and there's risk that it could be reused for some future purpose, so we have to keep the magic flag. :(
  • I tried to make the "how do we figure out the UART clock and/prescaler/transceiver" codepath for all the different ACPI ids easier to follow. There are some slight behavioral differences here-- ACPI device properties can technically now override clock frequency and prescaler with any of the ACPI IDs (and not just NIC7A69), but the earlier IDs shouldn't have any defined, so there should be no difference in practice.
  • There is documentation for the ni,ni16550 OF ID because checkpatch.pl complains about it. checkpatch.pl also complains if you don't put it in its own commit, which is why it's separated. I didn't actually try to give this a spin on a device using OF-based enumeration (aka Zynq-based cRIO) due to the effort involved in getting this version of the driver working with the current Zynq kernel.
  • Replaced the boilerplate license text with the SPDX identifier for that same text.

This is hopefully a lot cleaner and more upstream-acceptable-- but because it's a significant reworking, I'd like for it to simmer in our trees for a little bit to get some ATS runtime (in particular, get some testing on more than just a cRIO-904x with hand-testing with pyserial!). If it causes problems, we have a quick escape hatch (we can flip the kconfig token back)-- if it turns out good, then we can start tackling reverts on the older implementation.

You might also notice that "8250_ni" is named kind of generically-- I did consider trying to add support for PCI/PXI devices, at least to the extent of the since-reverted attempt in fdc2de8 ("serial/8250: Add support for NI-Serial PXI/PXIe+485 devices."). I think this is viable to add to 8250_ni (the RS-232 control register looks the same, so this mostly amounts to "add PCI enumeration and num_ports/address offsetting)), but wouldn't be a total replacement for the NI-Serial ni843x driver (lack of DMA). Maybe later...

I went with citing Jaeden and Karthik as the original authors-- most of the other changes since then have been mostly-trivial adding of IDs and/or handling the effects of 8250 core changes. Let me know if you think that should change.

@bstreiff
Copy link
Contributor Author

@ni/rtos

@bstreiff bstreiff requested a review from a team January 23, 2023 17:34
@bstreiff
Copy link
Contributor Author

also pinging @sjasonsmith who also expressed a desire to look at this but whom I cannot seem to add as a reviewer

@sjasonsmith
Copy link
Collaborator

The generic "transceiver interface" that was added in the earliest patches is now gone-- transceiver enable/disable just happens in the port startup/shutdown callbacks now. Having that as a separate API might have been nice, but I think it'd only be justifiable if I ported other 8250 drivers to use it

I think that when this was originally created there were not yet upstream mechanisms for RS-485 configuration. It's role decreased as it was merged with newer kernel versions, until finally reaching complete obsolescence in this PR.

Copy link

@gratian gratian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending @sjasonsmith approval before merging.

Copy link
Collaborator

@sjasonsmith sjasonsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve from my perspective. I can't verify that the details of all the different ACPI IDs is correct.

Wire modes will need to be revisited later.

@bstreiff bstreiff force-pushed the dev/bstreiff/6.0/ni16550 branch from 7a39588 to 75a6caa Compare January 31, 2023 18:07
@bstreiff
Copy link
Contributor Author

changes in v2:

  • clean up comments in header (it's not just a "transceiver driver")
  • use BIT macro
  • rename ni16550_config_rs485 -> ni16550_rs485_config to match struct member it is assigned to
  • remove 2-wire DTR with/without echo cases
  • don't update port->rs485 (uart_set_rs485_config does this for us)
  • use rs485_supported to declare support
  • program ACR for parity with PCI devices
  • enable/disable transceivers on all devices (also for parity with PCI)
  • mark the SPDX-License-Identifier in dt bindings actually as a SPDX-License-Identifier
  • add an extra commit to mark txvr_ops as deprecated (note AzDO#2280538)

@gratian gratian requested review from gratian and sjasonsmith and removed request for sjasonsmith February 2, 2023 22:58
@gratian
Copy link

gratian commented Feb 2, 2023

I've reset approvals in order to review v2.

Copy link
Collaborator

@sjasonsmith sjasonsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider my comments pretty minor, and I'm fine with what you decide is the proper reaction to them.

I'm not 100% of GitHub workflows. I'm marking this as "Request Changes" but my intent is that after you read my comments you can decide how to proceed and do so without having to wait on me.

The National Instruments (NI) 16550 is a standard 16550 with larger
FIFOs and embedded RS-232/RS-485 transceiver control circuitry. This
patch adds a driver that can operate this UART.

Originally by: Jaeden Amero <jaeden.amero@ni.com>
Originally by: Karthik Manamcheri <karthik.manamcheri@ni.com>

Modified extensively to move enumeration and other logic into a
self-contained platform_driver instead of being shoved into 8250_pnp.

Signed-off-by: Brenda Streiff <brenda.streiff@ni.com>
Add bindings for the NI 16550 UART.

Signed-off-by: Brenda Streiff <brenda.streiff@ni.com>
Switch to using the new 8250_ni UART implementation.

Signed-off-by: Brenda Streiff <brenda.streiff@ni.com>
We added an interface in 8e9e459 ("8250: Allow client drivers to
control tranceivers") for RS-485 transceiver control, but this never
went upstream and is also not a requirement for a client driver to
implement RS-485 transceiver control.

However, this out-of-tree interface got exposed to the NI-Serial drivers,
and they were using it for their driver for the NI 987x devices, so we
can't (yet) drop it outright.

Mark this interface as deprecated and emit a warning when registering a
client driver that intends to use it.

Upstream-Status: Inappropriate [NI-specific]
Signed-off-by: Brenda Streiff <brenda.streiff@ni.com>
@bstreiff bstreiff force-pushed the dev/bstreiff/6.0/ni16550 branch from 75a6caa to 840baca Compare February 6, 2023 19:34
@bstreiff
Copy link
Contributor Author

bstreiff commented Feb 6, 2023

v3 drops the rts delay stuff out of the rs485-supported struct.

@bstreiff bstreiff requested a review from sjasonsmith February 8, 2023 17:13
@bstreiff
Copy link
Contributor Author

Merged into nilrt/master/6.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants