Re: git: 1491fe8f864a - main - uart/pci: support 16550A PCI serial devices

On Thu, Apr 16, 2026 at 04:17:03PM -0400, Mark Johnston wrote:
> On Fri, Mar 27, 2026 at 08:33:37AM +0000, Roger Pau Monné wrote:
> > The branch main has been updated by royger:
> > 
> > URL: https://cgit.FreeBSD.org/src/commit/?id=1491fe8f864af5af37e83f1d12459905fb6097fd
> > 
> > commit 1491fe8f864af5af37e83f1d12459905fb6097fd
> > Author:     Roger Pau Monné <royger@FreeBSD.org>
> > AuthorDate: 2026-03-26 10:01:57 +0000
> > Commit:     Roger Pau Monné <royger@FreeBSD.org>
> > CommitDate: 2026-03-27 08:26:32 +0000
> > 
> >     uart/pci: support 16550A PCI serial devices
> >     
> >     Expand the current check to also attach the ns8250 driver to devices
> >     reporting as 16550A.  This has been tested to work on a real device.
> >     
> >     From an inspection of the code in the ns8250 driver it seems like it should
> >     support up to 16950A devices, but I don't have hardware to ensure that,
> >     hence be conservative with the change.
> 
> I have a system which hangs in uart_bus_probe() after this change.  It
> has a 2-port UART PCI card installed:
> 
> puc0@pci0:6:0:0:        class=0x070002 rev=0x00 hdr=0x00 vendor=0x1415 device=0xc158 subvendor=0x1415 subdevice=0xc158
>     vendor     = 'Oxford Semiconductor Ltd'
>     device     = 'OXPCIe952 Dual Native 950 UART'
>     class      = simple comms
>     subclass   = UART
> 
> Normally puc(4) attaches to it and creates two child uart devices:
> 
>     puc0
> 	Interrupt request lines:
> 	    0x11
> 	pcib7 memory window:
> 	    0x84000000-0x841fffff
> 	    0x84200000-0x843fffff
> 	    0x84400000-0x84403fff
>       uart2
> 	  puc0 I/O memory mapping:
> 	      0x84401000-0x844011ff
> 	  puc0 port numbers:
> 	      0x1
>       uart3
> 	  puc0 I/O memory mapping:
> 	      0x84401200-0x844013ff
> 	  puc0 port numbers:
> 	      0x2
> 
> After this change, though, uart(4) tries to probe the device directly
> and hangs.  Presumably we still want to exclude at least multi-port
> devices here?

Hm, I see, sorry, I didn't think changing the UART version
compatibility would cause this.  What about using BUS_PROBE_SPECIFIC
vs BUS_PROBE_GENERIC?

If the PCI device matches a device in the pci_ns8250_ids array we
return SPECIFIC, otherwise if it's a generic PCI serial device we
return GENERIC, thus allowing the puc driver to take over it.

Could you please give a try to the patch below.

Thanks, Roger.
---
diff --git a/sys/dev/uart/uart_bus_pci.c b/sys/dev/uart/uart_bus_pci.c
index b0d285e3c603..f8da4eaf8d25 100644
--- a/sys/dev/uart/uart_bus_pci.c
+++ b/sys/dev/uart/uart_bus_pci.c
@@ -280,33 +280,43 @@ uart_pci_probe(device_t dev)
 {
 	struct uart_softc *sc;
 	const struct pci_id *id;
-	struct pci_id cid = {
-		.regshft = 0,
-		.rclk = 0,
-		.rid = 0x10 | PCI_NO_MSI,
-		.desc = "Generic SimpleComm PCI device",
-	};
-	int result;
 
 	sc = device_get_softc(dev);
 
 	id = uart_pci_match(dev, pci_ns8250_ids);
 	if (id != NULL) {
 		sc->sc_class = &uart_ns8250_class;
-		goto match;
+		return BUS_PROBE_SPECIFIC;
 	}
 	if (pci_get_class(dev) == PCIC_SIMPLECOMM &&
 	    pci_get_subclass(dev) == PCIS_SIMPLECOMM_UART &&
 	    pci_get_progif(dev) <= PCIP_SIMPLECOMM_UART_16550A) {
-		/* XXX rclk what to do */
-		id = &cid;
 		sc->sc_class = &uart_ns8250_class;
-		goto match;
+		return BUS_PROBE_GENERIC;
 	}
 	/* Add checks for non-ns8250 IDs here. */
 	return (ENXIO);
+}
+
+static int
+uart_pci_attach(device_t dev)
+{
+	const struct pci_id cid = {
+		.regshft = 0,
+		.rclk = 0,
+		.rid = 0x10 | PCI_NO_MSI,
+		.desc = "Generic SimpleComm PCI device",
+	};
+	struct uart_softc *sc;
+	const struct pci_id *id = uart_pci_match(dev, pci_ns8250_ids);
+	int count, result;
+
+	if (id == NULL)
+		/* No specific PCI ID match, must be a generic device. */
+		id = &cid;
+
+	sc = device_get_softc(dev);
 
- match:
 	result = uart_bus_probe(dev, id->regshft, 0, id->rclk,
 	    id->rid & PCI_RID_MASK, 0, 0);
 	/* Bail out on error. */
@@ -322,26 +332,13 @@ uart_pci_probe(device_t dev)
 	/* Set/override the device description. */
 	if (id->desc)
 		device_set_desc(dev, id->desc);
-	return (result);
-}
-
-static int
-uart_pci_attach(device_t dev)
-{
-	struct uart_softc *sc;
-	const struct pci_id *id;
-	int count;
-
-	sc = device_get_softc(dev);
 
 	/*
-	 * Use MSI in preference to legacy IRQ if available. However, experience
-	 * suggests this is only reliable when one MSI vector is advertised.
+	 * Use MSI in preference to legacy IRQ if available. However,
+	 * experience suggests this is only reliable when one MSI vector is
+	 * advertised.
 	 */
-	id = uart_pci_match(dev, pci_ns8250_ids);
-	/* Always disable MSI for generic devices. */
-	if (id != NULL && (id->rid & PCI_NO_MSI) == 0 &&
-	    pci_msi_count(dev) == 1) {
+	if ((id->rid & PCI_NO_MSI) == 0 && pci_msi_count(dev) == 1) {
 		count = 1;
 		if (pci_alloc_msi(dev, &count) == 0) {
 			sc->sc_irid = 1;