Allow uart(4) to use MSI interrupts (for PCI child instances).
ClosedPublic
Actions

Authored by bms on Jan 10 2017, 7:41 PM.

Details

Reviewers

jhb
imp

Commits

rS311987: Allow uart(4) to use MSI interrupts on single-port PCI instances.

Summary

This is likely to aid users implementing 1PPS for NTP or PTP, however
it may also have benefits for hypervisor-emulated hardware.

Whilst this functionality would normally be implemented in puc(4),
the MCS9922 hardware generation appears to implement each UART with
discrete PCI configuration space (one function per UART).

So, puc(4) is not suitable, as it is not intended for single ports of this kind.
Indeed, its implementation actively disallows single ported drivers
from attaching under it (and returns ENXIO in its bus attach callback).

Test Plan

Tested with ASIX MCS9922 hardware: i.e. a Syba SD-PEX15022 dual PCIe UART card. vmstat -i reported that MSI interrupts were in use throughout.

lrzsz ZMODEM receives and sends were reliable in both directions up to 56K, using a separate Linux machine with an (also MCS9922) ExpressCard PCIe UART. Testing at 115K showed CRC errors, presumably due to the low quality DB9 null-modem cable in use.

Tested only with the PCIe slot variant -- it is entirely possible this works cleanly with the ExpressCard/34 product based on the same chip used by the test peer, but PCIe Hotplug is out of scope for this change.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bms updated this revision to Diff 23844.Jan 10 2017, 7:41 PM

bms retitled this revision from to Allow uart(4) to use MSI interrupts (for PCI child instances). This is likely to aid users implementing 1PPS for NTP or PTP, however it may also have benefits for hypervisor-emulated hardware..

bms updated this object.

bms edited the test plan for this revision. (Show Details)

bms added a reviewer: jhb.

Herald added a subscriber: imp. · View Herald TranscriptJan 10 2017, 7:41 PM

bms retitled this revision from Allow uart(4) to use MSI interrupts (for PCI child instances). This is likely to aid users implementing 1PPS for NTP or PTP, however it may also have benefits for hypervisor-emulated hardware. to Allow uart(4) to use MSI interrupts (for PCI child instances)..Jan 10 2017, 9:10 PM

bms updated this object.

bms edited the test plan for this revision. (Show Details)

bms edited the test plan for this revision. (Show Details)Jan 11 2017, 10:43 PM

imp accepted this revision.Jan 11 2017, 10:46 PM

imp added a reviewer: imp.

This revision is now accepted and ready to land.Jan 11 2017, 10:46 PM

rpokala added a subscriber: rpokala.Jan 11 2017, 11:20 PM

rpokala added inline comments.

sys/dev/uart/uart_bus_pci.c
224 ↗	(On Diff #23844)	This variable is initialized, but never used otherwise. Remove it?
250 ↗	(On Diff #23844)	This variable is initialized, but never used otherwise. Remove it?
sys/dev/uart/uart_core.c
684 ↗	(On Diff #23844)	While not strictly necessary, parentheses around the comparison would make this much easier to read, IMHO: RF_ACTIVE \| ((sc->sc_irid != 0) ? 0 : RF_SHAREABLE));

jhb added inline comments.Jan 11 2017, 11:28 PM

sys/dev/uart/uart_bus_pci.c
211 ↗	(On Diff #23844)	I would just call pci_msi_count() in uart_pci_attach() instead. I think you can then remove sc_msi entirely.
235 ↗	(On Diff #23844)	With a count of 1, you won't get back a count you don't expect. You can simplify this to just: if (pci_msi_count(dev) > 0) { count = 1; if (pci_alloc_msi(dev, &count == 0) { sc->sc_irid = 1; device_printf(...); } }
253 ↗	(On Diff #23844)	Then drop sc_msi from here and just use the check on sc_irid.
sys/dev/uart/uart_core.c
681 ↗	(On Diff #23844)	Just delete this line entirely. sc is a softc, so it's already initialized to 0. This lets bus callers (i.e. bus attachments) set sc_irid as needed (which PCI wants to do for MSI).
684 ↗	(On Diff #23844)	RF_SHAREABLE doesn't hurt for MSI, so I would just leave this line as is and always pass it like the current code does.