Re: kern/57359: 10.0_BETA failed to detect USB 2.0 Hub on AMD SB600

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

I wrote:

> >Number:         57359
> >Category:       kern
> >Synopsis:       10.0_BETA failed to detect USB 2.0 Hub on AMD SB600

 :

> >Description:
> On ASRock M3A UCC motherboard with AMD 480X CrossFire / AMD SB600
>  https://www.asrock.com/mb/AMD/M3A%20UCC/index.asp
> (no overclock, no unlock CPU Core)
> USB 2.0 Hub connected to an on-board USB port is not detected properly
> on NetBSD/i386 10.0_BETA GENERIC (202304150840Z):
> >> uhub3: autoconfiguration error: device problem, disabling port 6
> 
> while it works on NetBSD/i386 9.3 GENERIC:

After git-bisect(1) between netbsd-9-base and netbsd-10-base,
it turns out the following commit caused this SB600 ehci problem:

 https://mail-index.netbsd.org/source-changes/2020/05/05/msg117061.html

---
Module Name:    src
Committed By:   bouyer
Date:           Tue May  5 16:58:11 UTC 2020

Modified Files:
        src/sys/dev/pci: pci_map.c

Log Message:
disable I/O or mem decode before probing BAR size.
Bar size is probed writing 0xffffffff to the BAR and reading back; but while
doing this the decoding address is not guaranteed to be valid and could have
side effect.

Xen PVH enforces disabling decoding before writing to a BAR.

Proposed on tech-kern@, got positive comments


To generate a diff of this commit:
cvs rdiff -u -r1.39 -r1.40 src/sys/dev/pci/pci_map.c

```
Index: src/sys/dev/pci/pci_map.c
diff -u src/sys/dev/pci/pci_map.c:1.39 src/sys/dev/pci/pci_map.c:1.40
--- src/sys/dev/pci/pci_map.c:1.39	Mon Dec  2 17:13:13 2019
+++ src/sys/dev/pci/pci_map.c	Tue May  5 16:58:11 2020
@@ -1,4 +1,4 @@
-/*	$NetBSD: pci_map.c,v 1.39 2019/12/02 17:13:13 riastradh Exp $	*/
+/*	$NetBSD: pci_map.c,v 1.40 2020/05/05 16:58:11 bouyer Exp $	*/
 
 /*-
  * Copyright (c) 1998, 2000 The NetBSD Foundation, Inc.
@@ -34,7 +34,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: pci_map.c,v 1.39 2019/12/02 17:13:13 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: pci_map.c,v 1.40 2020/05/05 16:58:11 bouyer Exp $");
 
 #include <sys/param.h>
 #include <sys/systm.h>
@@ -49,7 +49,7 @@
 pci_io_find(pci_chipset_tag_t pc, pcitag_t tag, int reg, pcireg_t type,
     bus_addr_t *basep, bus_size_t *sizep, int *flagsp)
 {
-	pcireg_t address, mask;
+	pcireg_t address, mask, csr;
 	int s;
 
 	if (reg < PCI_MAPREG_START ||
@@ -75,9 +75,18 @@
 	 */
 	s = splhigh();
 	address = pci_conf_read(pc, tag, reg);
+	/*
+	 * Disable decoding via the command register before writing to the
+	 * BAR register. Changing the decoding address to all-one is
+	 * not a valid address and could have side effects.
+	 */
+	csr = pci_conf_read(pc, tag, PCI_COMMAND_STATUS_REG);
+	pci_conf_write(pc, tag, PCI_COMMAND_STATUS_REG,
+	    csr & ~PCI_COMMAND_IO_ENABLE) ;
 	pci_conf_write(pc, tag, reg, 0xffffffff);
 	mask = pci_conf_read(pc, tag, reg);
 	pci_conf_write(pc, tag, reg, address);
+	pci_conf_write(pc, tag, PCI_COMMAND_STATUS_REG, csr);
 	splx(s);
 
 	if (PCI_MAPREG_TYPE(address) != PCI_MAPREG_TYPE_IO) {
@@ -107,6 +116,7 @@
 	pcireg_t address, mask, address1 = 0, mask1 = 0xffffffff;
 	uint64_t waddress, wmask;
 	int s, is64bit, isrom;
+	pcireg_t csr;
 
 	is64bit = (PCI_MAPREG_MEM_TYPE(type) == PCI_MAPREG_MEM_TYPE_64BIT);
 	isrom = (reg == PCI_MAPREG_ROM);
@@ -138,6 +148,14 @@
 	 */
 	s = splhigh();
 	address = pci_conf_read(pc, tag, reg);
+	csr = pci_conf_read(pc, tag, PCI_COMMAND_STATUS_REG);
+	/*
+	 * Disable decoding via the command register before writing to the
+	 * BAR register. Changing the decoding address to all-one is
+	 * not a valid address and could have side effects.
+	 */
+	pci_conf_write(pc, tag, PCI_COMMAND_STATUS_REG,
+	    csr & ~PCI_COMMAND_MEM_ENABLE) ;
 	pci_conf_write(pc, tag, reg, 0xffffffff);
 	mask = pci_conf_read(pc, tag, reg);
 	pci_conf_write(pc, tag, reg, address);
@@ -149,6 +167,7 @@
 			pci_conf_write(pc, tag, reg + 4, address1);
 		}
 	}
+	pci_conf_write(pc, tag, PCI_COMMAND_STATUS_REG, csr);
 	splx(s);
 
 	if (!isrom) {
@@ -240,14 +259,28 @@
 int
 pci_mapreg_probe(pci_chipset_tag_t pc, pcitag_t tag, int reg, pcireg_t *typep)
 {
-	pcireg_t address, mask;
+	pcireg_t address, mask, csr;
 	int s;
 
 	s = splhigh();
 	address = pci_conf_read(pc, tag, reg);
+	/*
+	 * Disable decoding via the command register before writing to the
+	 * BAR register. Changing the decoding address to all-one is
+	 * not a valid address and could have side effects.
+	 */
+	csr = pci_conf_read(pc, tag, PCI_COMMAND_STATUS_REG);
+	if (PCI_MAPREG_TYPE(address) == PCI_MAPREG_TYPE_IO) {
+		pci_conf_write(pc, tag, PCI_COMMAND_STATUS_REG,
+		    csr & ~PCI_COMMAND_IO_ENABLE);
+	} else {
+		pci_conf_write(pc, tag, PCI_COMMAND_STATUS_REG,
+		    csr & ~PCI_COMMAND_MEM_ENABLE);
+	}
 	pci_conf_write(pc, tag, reg, 0xffffffff);
 	mask = pci_conf_read(pc, tag, reg);
 	pci_conf_write(pc, tag, reg, address);
+	pci_conf_write(pc, tag, PCI_COMMAND_STATUS_REG, csr);
 	splx(s);
 
 	if (mask == 0) /* unimplemented mapping register */
```

Actually reverting this change on netbsd-10 branch makes USB 2.0
devices on ehci(4) work again on my M3A-UCC.

However, as noted in the above log the changes in pci_map.c rev 1.40
seem correct per the PCI specification, so the problem in this
PR could be caused by some SB600 specific side effects around
PCI_COMMAND_STATUS_REG accesses.

After several try-and-errors, it works around to move existing
SB600/SB700 quirk handling in ehci_pci.c (writing a MAGIC value
to SBx00 specific EHCI_SBx00_WORKAROUND_REG) after the enabling
devices even with pci_map.c rev 1.40, at least on my ASRock M3A-UCC.

```
Index: sys/dev/pci/ehci_pci.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/ehci_pci.c,v
retrieving revision 1.75.2.2
diff -u -p -d -r1.75.2.2 ehci_pci.c
--- sys/dev/pci/ehci_pci.c	11 Sep 2024 15:56:21 -0000	1.75.2.2
+++ sys/dev/pci/ehci_pci.c	4 Jan 2026 16:59:08 -0000
@@ -168,6 +168,15 @@ ehci_pci_attach(device_t parent, device_
 	DPRINTF(("%s: offs=%d\n", device_xname(self), sc->sc.sc_offs));
 	EOWRITE4(&sc->sc, EHCI_USBINTR, 0);
 
+	pcireg_t intr = pci_conf_read(pc, tag, PCI_INTERRUPT_REG);
+	int pin = PCI_INTERRUPT_PIN(intr);
+
+	/* Enable the device. */
+	csr = pci_conf_read(pc, tag, PCI_COMMAND_STATUS_REG);
+	csr |= PCI_COMMAND_MASTER_ENABLE;
+	csr &= ~(pin ? PCI_COMMAND_INTERRUPT_DISABLE : 0);
+	pci_conf_write(pc, tag, PCI_COMMAND_STATUS_REG, csr);
+
 	/* Handle quirks */
 	switch (quirk) {
 	case EHCI_PCI_QUIRK_AMD_SB600:
@@ -179,15 +188,6 @@ ehci_pci_attach(device_t parent, device_
 		break;
 	}
 
-	pcireg_t intr = pci_conf_read(pc, tag, PCI_INTERRUPT_REG);
-	int pin = PCI_INTERRUPT_PIN(intr);
-
-	/* Enable the device. */
-	csr = pci_conf_read(pc, tag, PCI_COMMAND_STATUS_REG);
-	csr |= PCI_COMMAND_MASTER_ENABLE;
-	csr &= ~(pin ? PCI_COMMAND_INTERRUPT_DISABLE : 0);
-	pci_conf_write(pc, tag, PCI_COMMAND_STATUS_REG, csr);
-
 	/* Map and establish the interrupt. */
 	if (pci_intr_alloc(pa, &sc->sc_pihp, NULL, 0) != 0) {
 		aprint_error_dev(self, "couldn't map interrupt\n");
```

This chane should not affect other systems than SB600 (and early
SB700) systems, so if there is no particular objection I'll commit
the above fix soon.

Thanks,
---
Izumi Tsutsui