Discussion:
help updating EHCI driver
Add Reply
Paul B. Henson
2017-12-06 02:48:32 UTC
Reply
Permalink
Raw Message
I'm trying to port some quirks for AMD USB chipsets from other operating
systems to OpenBSD to hopefully resolve issues I am having with the pc
engines APU3 EHCI ports, as they seem to work fine on those systems.
I've got a pretty rough draft of one of them, which disables low-power
mode during transfers, but would appreciate a little clarification on
device I/O as I'm not generally a device driver developer.

Under Linux, the kernel uses absolute addresses when it's doing port I/O
to a device, so that's what I am referencing in their implementation. In
OpenBSD I see that a driver maps a handle to a region of memory and then
uses offsets from the base of that region for port I/O. It looks like
the EHCI driver code has already mapped that region and the handle is
available for me to use, but I don't see where that mapping was made or
how to figure out what the base was in order to turn the absolute
addresses I have into appropriate offsets to use with the openbsd API?

Then, for some of the chipsets, in addition to poking at the USB device
itself to twiddle the low-power mode, you also have to muck with the
northbridge configuration. I think I gathered the device information,
although I don't know that was the correct way to do so; but I need to
map the I/O region for it to a handle so I can modify it. If a driver for
one device needs to write to a different device is it supposed to call
bus_space_map on its own to get a mapping, or can it somehow get access
to the existing one already in place for that device?

Finally, low power mode is supposed to be disabled whenever there are
asynchronous transfers occurring, and then re-enabled once they
complete. I'm not sure I've put the calls in the right place, and I know
I haven't handled the case where transfers fail or are canceled rather
than complete.

The other quirk involves never having an empty frame list; I have
implemented the logic to detect when that is required, but haven't even
come close to wrapping my head around actually implementing the quirk
itself.

In any case, here is my current laughable diff, advice and corrections
most appreciated.


Index: pci/ehci_pci.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/ehci_pci.c,v
retrieving revision 1.30
diff -u -p -r1.30 ehci_pci.c
--- pci/ehci_pci.c 20 Jul 2016 09:48:06 -0000 1.30
+++ pci/ehci_pci.c 6 Dec 2017 02:46:24 -0000
@@ -66,6 +66,8 @@ struct ehci_pci_softc {
};

int ehci_sb700_match(struct pci_attach_args *pa);
+int ehci_amd_pll_quirk_match(struct pci_attach_args *pa);
+int ehci_amd_pll_quirk_match_nb(struct pci_attach_args *pa);

#define EHCI_SBx00_WORKAROUND_REG 0x50
#define EHCI_SBx00_WORKAROUND_ENABLE (1 << 3)
@@ -111,6 +113,7 @@ ehci_pci_attach(struct device *parent, s
char *devname = sc->sc.sc_bus.bdev.dv_xname;
usbd_status r;
int s;
+ struct pci_attach_args amd_pa;

/* Map I/O registers */
if (pci_mapreg_map(pa, PCI_CBMEM, PCI_MAPREG_TYPE_MEM, 0,
@@ -131,6 +134,86 @@ ehci_pci_attach(struct device *parent, s

/* Handle quirks */
switch (PCI_VENDOR(pa->pa_id)) {
+ case PCI_VENDOR_AMD:
+ /* AMD errata indicates 8111 chipset EHCI is broken */
+ if (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_8111_EHCI) {
+ printf("%s: AMD 8111 EHCI broken, skipping", devname);
+ goto disestablish_ret;
+ }
+ if (pci_find_device(&amd_pa, ehci_amd_pll_quirk_match)) {
+ sc->sc.amd_chipset.rev = PCI_REVISION(amd_pa.pa_class);
+ if (PCI_PRODUCT(amd_pa.pa_id) == PCI_PRODUCT_ATI_SBX00_SMB) {
+ if (sc->sc.amd_chipset.rev >= 0x10 &&
+ sc->sc.amd_chipset.rev <= 0x1f)
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_SB600;
+ else if (sc->sc.amd_chipset.rev >= 0x30 &&
+ sc->sc.amd_chipset.rev <= 0x3f)
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_SB700;
+ else if (sc->sc.amd_chipset.rev >= 0x40 &&
+ sc->sc.amd_chipset.rev <= 0x4f)
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_SB800;
+ else
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_UNKNOWN;
+
+ }
+ else if (PCI_PRODUCT(amd_pa.pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB) {
+ if (sc->sc.amd_chipset.rev >= 0x11 &&
+ sc->sc.amd_chipset.rev <= 0x14)
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_HUDSON2;
+ else if (sc->sc.amd_chipset.rev >= 0x15 &&
+ sc->sc.amd_chipset.rev <= 0x18)
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_BOLTON;
+ else if (sc->sc.amd_chipset.rev >= 0x39 &&
+ sc->sc.amd_chipset.rev <= 0x3a)
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_YANGTZE;
+ else
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_UNKNOWN;
+ }
+
+ if ((sc->sc.amd_chipset.gen == AMD_CHIPSET_SB700 &&
+ sc->sc.amd_chipset.rev <= 0x3b) ||
+ sc->sc.amd_chipset.gen == AMD_CHIPSET_SB800 ||
+ sc->sc.amd_chipset.gen == AMD_CHIPSET_HUDSON2 ||
+ sc->sc.amd_chipset.gen == AMD_CHIPSET_BOLTON) {
+
+ DPRINTF(("%s: enabling AMD PLL quirk workaround\n", devname));
+ sc->sc.sc_flags |= EHCIF_AMD_PLL_QUIRK;
+
+ if (pci_find_device(&amd_pa, ehci_amd_pll_quirk_match_nb)) {
+ sc->sc.amd_chipset.nb_pa_iot = amd_pa.pa_iot;
+ sc->sc.amd_chipset.nb_pa_pc = amd_pa.pa_pc;
+ sc->sc.amd_chipset.nb_pa_tag = amd_pa.pa_tag;
+ switch(PCI_PRODUCT(amd_pa.pd_id)) {
+ case PCI_PRODUCT_AMD_RS880_HB:
+ sc->sc.amd_chipset.nb_type = 1;
+ break;
+ case PCI_PRODUCT_AMD_AMD64_14_HB:
+ sc->sc.amd_chipset.nb_type = 2;
+ break;
+ case PCI_PRODUCT_AMD_RS780_HB:
+ sc->sc.amd_chipset.nb_type = 3;
+ break;
+ }
+ /* XXX How to map this? */
+ if (bus_space_map(sc->sc.amd_chipset.nb_pa_iot, XXX, XXX, 0, &amd_chipset.nb_io_handle)) {
+ printf("%s: failed to map northbridge IO space, skipping AMD pll quirk\n");
+ sc->sc.sc_flags &= ~EHCIF_AMD_PLL_QUIRK;
+ }
+ }
+ else
+ sc->sc.amd_chipset.nb_type = 0;
+ }
+ }
+ /* When there is a NULL pointer with T-bit set to 1 in the frame list
+ table the Hudson-2 chipset might read/write memory that doesn't belong
+ to it; the frame list link pointer should always have a valid pointer
+ to a inactive qh to avoid this */
+ if (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_EHCI) {
+ DPRINTF(("%s: enabling AMD Hudson-2 dummy qh workaround\n", devname));
+ sc->sc.sc_flags |= EHCIF_AMD_DUMMY_QH_QUIRK;
+ }
+
+ break;
case PCI_VENDOR_ATI:
if (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_ATI_SB600_EHCI ||
(PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_ATI_SB700_EHCI &&
@@ -342,4 +425,157 @@ ehci_sb700_match(struct pci_attach_args
return (1);

return (0);
+}
+
+/* Check for revisions of AMD USB chipsets that need this quirk applied */
+int
+ehci_amd_pll_quirk_match(struct pci_attach_args *pa)
+{
+ if (PCI_VENDOR(pa->pa_id) == PCI_VENDOR_ATI &&
+ (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_ATI_SBX00_SMB ||
+ PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB))
+ return 1;
+
+ return 0;
+}
+
+/* If amd pll quirk is required, find northbridge device */
+int
+ehci_amd_pll_quirk_match_nb(struct pci_attach_args *pa)
+{
+ if (PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD &&
+ (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_RS880_HB ||
+ PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_AMD64_14_HB ||
+ PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_RS780_HB))
+ return 1;
+
+ return 0;
+}
+
+void usb_amd_pll_quirk(struct ehci_softc *sc, int enable) {
+ u_int32_t addr, addr_low, addr_high, val;
+ u_int32_t bit = enable ? 1 : 0;
+
+ DPRINTF(("usb_amd_pll_quirk: %d\n", enable));
+
+ if (enable) {
+ sc->amd_chipset.isoc_count++;
+ if (sc->amd_chipset.isoc_count > 1)
+ return;
+ }
+ else {
+ sc->amd_chipset.isoc_count--;
+ if (sc->amd_chipset.isoc_count > 0)
+ return;
+ }
+
+ if (sc->amd_chipset.gen == AMD_CHIPSET_SB800 ||
+ sc->amd_chipset.gen == AMD_CHIPSET_HUDSON2 ||
+ sc->amd_chipset.sb_type.gen == AMD_CHIPSET_BOLTON) {
+ /* XXX These addresses are absolute from linux code, need to be offsets here? */
+ EWRITE1(sc, 0xcd6, 0xe0);
+ addr_low = EREAD1(sc, 0xcd7);
+ EWRITE1(sc, 0xcd6, 0xe1);
+ addr_high = EREAD1(sc, 0xcd7);
+ addr = addr_high << 8 | addr_low;
+ EWRITE4(sc, addr, 0x30);
+ EWRITE4(sc, addr+0x04, 0x40);
+ EWRITE4(sc, addr, 0x34);
+ val = EREAD4(addr+0x04);
+ if (enable) {
+ val &= ~0x08;
+ val |= (1 << 4) | (1 << 9);
+ }
+ else {
+ val |= 0x08;
+ val &= ~((1 << 4) | (1 << 9));
+ }
+ EWRITE4(sc, addr+0x04, val);
+ }
+ else if (sc->amd_chipset.gen == AMD_CHIPSET_SB700 &&
+ sc->amd_chipset.rev <= 0x3b) {
+ addr = pci_conf_read(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xf0);
+ /* XXX These addresses are absolute, should be offsets? */
+ bus_space_write_4(sc->amd_chipset.nb_pa_iot, sc->amd_chipset.nb_io_handle,
+ addr, 0x30)
+ bus_space_write_4(sc->amd_chipset.nb_pa_iot, sc->amd_chipset.nb_io_handle,
+ addr+0x04, 0x40);
+ bus_space_write_4(sc->amd_chipset.nb_pa_iot, sc->amd_chipset.nb_io_handle,
+ addr, 0x34);
+ val = bus_space_read_4(sc->amd_chipset.nb_pa_iot, sc->amd_chipset.nb_io_handle,
+ addr+0x04);
+ if (enable) {
+ val &= ~0x08;
+ val |= (1 << 4) | (1 << 9);
+ }
+ else {
+ val |= 0x08;
+ val &= ~((1 << 4) | (1 << 9));
+ }
+ bus_space_write_4(sc->amd_chipset.nb_pa_iot, sc->amd_chipset.nb_io_handle,
+ addr+0x04, val);
+ }
+ else
+ return;
+
+ if (!sc->amd_chipset.nb_dev) {
+ return;
+ }
+
+ if (sc->amd_chipset.nb_type == 1 || sc->amd_chipset.nb_type == 3) {
+
+ /* XXX These addresses are absolute, should be offsets? */
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe0, 0x10040);
+ val = pci_conf_read(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4);
+
+ val &= ~(1 | (1 << 3) | (1 << 4) | (1 << 9) | (1 << 12));
+ val |= bit | (bit << 3) | (bit << 12);
+ val |= ((!bit) << 4) | ((!bit) << 9);
+
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4, val);
+
+
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe0, 0x10002);
+ val = pci_conf_read(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4);
+
+ val &= ~(1 << 8);
+ val |= bit << 8;
+
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4, val);
+ }
+ else if (sc->amd_chipset.nb_type == 2) {
+ /* XXX These addresses are absolute, should be offsets? */
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe0, 0x01100012);
+ val = pci_conf_read(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4);
+
+ if (disable)
+ val &= ~(0x3f << 7);
+ else
+ val |= 0x3f << 7;
+
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4, val);
+
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe0, 0x01100013);
+ val = pci_conf_read(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4);
+*/
+ if (disable)
+ val &= ~(0x3f << 7);
+ else
+ val |= 0x3f << 7;
+
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4, val);
+ }
}
Index: usb/ehci.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.200
diff -u -p -r1.200 ehci.c
--- usb/ehci.c 15 May 2017 10:52:08 -0000 1.200
+++ usb/ehci.c 6 Dec 2017 02:46:24 -0000
@@ -81,10 +81,11 @@ struct cfdriver ehci_cd = {
NULL, "ehci", DV_DULL
};

+#define EHCI_DEBUG
#ifdef EHCI_DEBUG
#define DPRINTF(x) do { if (ehcidebug) printf x; } while(0)
#define DPRINTFN(n,x) do { if (ehcidebug>(n)) printf x; } while (0)
-int ehcidebug = 0;
+int ehcidebug = 10;
#define bitmask_snprintf(q,f,b,l) snprintf((b), (l), "%b", (q), (f))
#else
#define DPRINTF(x)
@@ -3351,6 +3352,8 @@ ehci_device_isoc_start(struct usbd_xfer
TAILQ_INSERT_TAIL(&sc->sc_intrhead, ex, inext);
xfer->status = USBD_IN_PROGRESS;
xfer->done = 0;
+ if (sc->sc.sc_flags && EHCIF_AMD_PLL_QUIRK)
+ usb_amd_pll_quirk(sc, 1);
splx(s);

return (USBD_IN_PROGRESS);
@@ -3587,6 +3590,8 @@ ehci_device_isoc_done(struct usbd_xfer *

s = splusb();
epipe->u.isoc.cur_xfers--;
+ if (sc->sc.sc_flags && EHCIF_AMD_PLL_QUIRK)
+ usb_amd_pll_quirk(sc, 0);
if (xfer->status != USBD_NOMEM) {
ehci_rem_itd_chain(sc, ex);
ehci_free_itd_chain(sc, ex);
Index: usb/ehcivar.h
===================================================================
RCS file: /cvs/src/sys/dev/usb/ehcivar.h,v
retrieving revision 1.37
diff -u -p -r1.37 ehcivar.h
--- usb/ehcivar.h 2 Oct 2016 06:36:39 -0000 1.37
+++ usb/ehcivar.h 6 Dec 2017 02:46:24 -0000
@@ -121,6 +121,28 @@ struct ehci_soft_islot {

#define EHCI_FREE_LIST_INTERVAL 100

+/* Store info about amd chipset for workarounds */
+enum amd_chipset_gen {
+ NOT_AMD_CHIPSET = 0,
+ AMD_CHIPSET_SB600,
+ AMD_CHIPSET_SB700,
+ AMD_CHIPSET_SB800,
+ AMD_CHIPSET_HUDSON2,
+ AMD_CHIPSET_BOLTON,
+ AMD_CHIPSET_YANGTZE,
+ AMD_CHIPSET_UNKNOWN
+};
+struct amd_chipset_data {
+ int nb_type;
+ bus_space_tag_t nb_pa_iot;
+ bus_space_handle_t nb_io_handle;
+ pci_chipset_tag_t nb_pa_pc;
+ pcitag_t nb_pa_tag;
+ enum amd_chipset_gen gen;
+ int rev;
+ int isoc_count;
+};
+
struct ehci_softc {
struct usbd_bus sc_bus; /* base device */
bus_space_tag_t iot;
@@ -131,6 +153,8 @@ struct ehci_softc {
#define EHCIF_DROPPED_INTR_WORKAROUND 0x01
#define EHCIF_PCB_INTR 0x02
#define EHCIF_USBMODE 0x04
+#define EHCIF_AMD_PLL_QUIRK 0x08
+#define EHCIF_AMD_DUMMY_QH_QUIRK 0x10

char sc_vendor[16]; /* vendor string for root hub */
int sc_id_vendor; /* vendor ID for root hub */
@@ -164,6 +188,8 @@ struct ehci_softc {
struct rwlock sc_doorbell_lock;

struct timeout sc_tmo_intrlist;
+
+ struct amd_chipset_data amd_chipset;
};

#define EREAD1(sc, a) bus_space_read_1((sc)->iot, (sc)->ioh, (a))
Martin Pieuchot
2017-12-07 11:17:31 UTC
Reply
Permalink
Raw Message
Post by Paul B. Henson
I'm trying to port some quirks for AMD USB chipsets from other operating
systems to OpenBSD to hopefully resolve issues I am having with the pc
engines APU3 EHCI ports, as they seem to work fine on those systems.
Which issue are you having? What makes you think that the quirks below
will help? What do you mean with 'work fine on those systems'? If they
work fine, which issues are you having?
Post by Paul B. Henson
I've got a pretty rough draft of one of them, which disables low-power
mode during transfers, but would appreciate a little clarification on
device I/O as I'm not generally a device driver developer.
Under Linux, the kernel uses absolute addresses when it's doing port I/O
to a device, so that's what I am referencing in their implementation. In
OpenBSD I see that a driver maps a handle to a region of memory and then
uses offsets from the base of that region for port I/O. It looks like
the EHCI driver code has already mapped that region and the handle is
available for me to use, but I don't see where that mapping was made or
how to figure out what the base was in order to turn the absolute
addresses I have into appropriate offsets to use with the openbsd API?
Then, for some of the chipsets, in addition to poking at the USB device
itself to twiddle the low-power mode, you also have to muck with the
northbridge configuration. I think I gathered the device information,
although I don't know that was the correct way to do so; but I need to
map the I/O region for it to a handle so I can modify it. If a driver for
one device needs to write to a different device is it supposed to call
bus_space_map on its own to get a mapping, or can it somehow get access
to the existing one already in place for that device?
It depends how the controller is connected to the host. If you look at
the PCI glue driver, dev/pci/ehci_pci.c you'll see

115: /* Map I/O registers */
116: if (pci_mapreg_map(pa, PCI_CBMEM, PCI_MAPREG_TYPE_MEM, 0,
117: &sc->sc.iot, &sc->sc.ioh, NULL, &sc->sc.sc_size, 0))

Then in the EHCI driver, dev/usb/ehci.c, these are accessed via the
EREAD/EWRITE/EOREAD/EOWRITE macros.

But maybe you just want to use pci_conf_read()/pci_conf_write()?
Post by Paul B. Henson
Finally, low power mode is supposed to be disabled whenever there are
asynchronous transfers occurring, and then re-enabled once they
complete. I'm not sure I've put the calls in the right place, and I know
I haven't handled the case where transfers fail or are canceled rather
than complete.
Is low power mode enabled on OpenBSD?
Post by Paul B. Henson
The other quirk involves never having an empty frame list; I have
implemented the logic to detect when that is required, but haven't even
come close to wrapping my head around actually implementing the quirk
itself.
For which transfer type is this quirk required, isochronous only?
Post by Paul B. Henson
In any case, here is my current laughable diff, advice and corrections
most appreciated.
Thanks for sending a diff. Don't hesitate to send them to tech@ next
time.

Comments inline.
Post by Paul B. Henson
Index: pci/ehci_pci.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/ehci_pci.c,v
retrieving revision 1.30
diff -u -p -r1.30 ehci_pci.c
--- pci/ehci_pci.c 20 Jul 2016 09:48:06 -0000 1.30
+++ pci/ehci_pci.c 6 Dec 2017 02:46:24 -0000
@@ -66,6 +66,8 @@ struct ehci_pci_softc {
};
int ehci_sb700_match(struct pci_attach_args *pa);
+int ehci_amd_pll_quirk_match(struct pci_attach_args *pa);
+int ehci_amd_pll_quirk_match_nb(struct pci_attach_args *pa);
#define EHCI_SBx00_WORKAROUND_REG 0x50
#define EHCI_SBx00_WORKAROUND_ENABLE (1 << 3)
@@ -111,6 +113,7 @@ ehci_pci_attach(struct device *parent, s
char *devname = sc->sc.sc_bus.bdev.dv_xname;
usbd_status r;
int s;
+ struct pci_attach_args amd_pa;
/* Map I/O registers */
if (pci_mapreg_map(pa, PCI_CBMEM, PCI_MAPREG_TYPE_MEM, 0,
@@ -131,6 +134,86 @@ ehci_pci_attach(struct device *parent, s
/* Handle quirks */
switch (PCI_VENDOR(pa->pa_id)) {
+ /* AMD errata indicates 8111 chipset EHCI is broken */
+ if (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_8111_EHCI) {
+ printf("%s: AMD 8111 EHCI broken, skipping", devname);
+ goto disestablish_ret;
If it is broken we should not match it. That means ehci_pci_match()
should contain a check for this model an return 0 instead of 1.

This could be a diff on its own, but please make sure to give a
reference for where you found the piece of information. A vendor
document is always the best, does it exist an errata for this chip?
At least the Linux commit hash such that it makes reviewer life easy ;)
Post by Paul B. Henson
+ if (pci_find_device(&amd_pa, ehci_amd_pll_quirk_match)) {
+ sc->sc.amd_chipset.rev = PCI_REVISION(amd_pa.pa_class);
+ if (PCI_PRODUCT(amd_pa.pa_id) == PCI_PRODUCT_ATI_SBX00_SMB) {
+ if (sc->sc.amd_chipset.rev >= 0x10 &&
+ sc->sc.amd_chipset.rev <= 0x1f)
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_SB600;
+ else if (sc->sc.amd_chipset.rev >= 0x30 &&
+ sc->sc.amd_chipset.rev <= 0x3f)
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_SB700;
+ else if (sc->sc.amd_chipset.rev >= 0x40 &&
+ sc->sc.amd_chipset.rev <= 0x4f)
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_SB800;
+ else
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_UNKNOWN;
+
You don't need to reset the value, just read the code in this file, you
can use the PCI_REVISION() and PCI_PRODUCT() macro ;)
Post by Paul B. Henson
+ }
+ else if (PCI_PRODUCT(amd_pa.pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB) {
+ if (sc->sc.amd_chipset.rev >= 0x11 &&
+ sc->sc.amd_chipset.rev <= 0x14)
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_HUDSON2;
+ else if (sc->sc.amd_chipset.rev >= 0x15 &&
+ sc->sc.amd_chipset.rev <= 0x18)
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_BOLTON;
+ else if (sc->sc.amd_chipset.rev >= 0x39 &&
+ sc->sc.amd_chipset.rev <= 0x3a)
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_YANGTZE;
+ else
+ sc->sc.amd_chipset.gen = AMD_CHIPSET_UNKNOWN;
+ }
+
+ if ((sc->sc.amd_chipset.gen == AMD_CHIPSET_SB700 &&
+ sc->sc.amd_chipset.rev <= 0x3b) ||
+ sc->sc.amd_chipset.gen == AMD_CHIPSET_SB800 ||
+ sc->sc.amd_chipset.gen == AMD_CHIPSET_HUDSON2 ||
+ sc->sc.amd_chipset.gen == AMD_CHIPSET_BOLTON) {
+
+ DPRINTF(("%s: enabling AMD PLL quirk workaround\n", devname));
+ sc->sc.sc_flags |= EHCIF_AMD_PLL_QUIRK;
+
+ if (pci_find_device(&amd_pa, ehci_amd_pll_quirk_match_nb)) {
+ sc->sc.amd_chipset.nb_pa_iot = amd_pa.pa_iot;
+ sc->sc.amd_chipset.nb_pa_pc = amd_pa.pa_pc;
+ sc->sc.amd_chipset.nb_pa_tag = amd_pa.pa_tag;
+ switch(PCI_PRODUCT(amd_pa.pd_id)) {
+ sc->sc.amd_chipset.nb_type = 1;
+ break;
+ sc->sc.amd_chipset.nb_type = 2;
+ break;
+ sc->sc.amd_chipset.nb_type = 3;
+ break;
+ }
+ /* XXX How to map this? */
+ if (bus_space_map(sc->sc.amd_chipset.nb_pa_iot, XXX, XXX, 0, &amd_chipset.nb_io_handle)) {
+ printf("%s: failed to map northbridge IO space, skipping AMD pll quirk\n");
+ sc->sc.sc_flags &= ~EHCIF_AMD_PLL_QUIRK;
+ }
+ }
+ else
+ sc->sc.amd_chipset.nb_type = 0;
+ }
+ }
+ /* When there is a NULL pointer with T-bit set to 1 in the frame list
+ table the Hudson-2 chipset might read/write memory that doesn't belong
+ to it; the frame list link pointer should always have a valid pointer
+ to a inactive qh to avoid this */
+ if (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_EHCI) {
+ DPRINTF(("%s: enabling AMD Hudson-2 dummy qh workaround\n", devname));
+ sc->sc.sc_flags |= EHCIF_AMD_DUMMY_QH_QUIRK;
+ }
+
+ break;
if (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_ATI_SB600_EHCI ||
(PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_ATI_SB700_EHCI &&
@@ -342,4 +425,157 @@ ehci_sb700_match(struct pci_attach_args
return (1);
return (0);
+}
+
+/* Check for revisions of AMD USB chipsets that need this quirk applied */
+int
+ehci_amd_pll_quirk_match(struct pci_attach_args *pa)
+{
+ if (PCI_VENDOR(pa->pa_id) == PCI_VENDOR_ATI &&
+ (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_ATI_SBX00_SMB ||
+ PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB))
+ return 1;
+
+ return 0;
+}
+
+/* If amd pll quirk is required, find northbridge device */
+int
+ehci_amd_pll_quirk_match_nb(struct pci_attach_args *pa)
+{
+ if (PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD &&
+ (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_RS880_HB ||
+ PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_AMD64_14_HB ||
+ PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_RS780_HB))
+ return 1;
+
+ return 0;
+}
+
+void usb_amd_pll_quirk(struct ehci_softc *sc, int enable) {
+ u_int32_t addr, addr_low, addr_high, val;
+ u_int32_t bit = enable ? 1 : 0;
+
+ DPRINTF(("usb_amd_pll_quirk: %d\n", enable));
+
+ if (enable) {
+ sc->amd_chipset.isoc_count++;
+ if (sc->amd_chipset.isoc_count > 1)
+ return;
+ }
+ else {
+ sc->amd_chipset.isoc_count--;
+ if (sc->amd_chipset.isoc_count > 0)
+ return;
+ }
+
+ if (sc->amd_chipset.gen == AMD_CHIPSET_SB800 ||
+ sc->amd_chipset.gen == AMD_CHIPSET_HUDSON2 ||
+ sc->amd_chipset.sb_type.gen == AMD_CHIPSET_BOLTON) {
+ /* XXX These addresses are absolute from linux code, need to be offsets here? */
+ EWRITE1(sc, 0xcd6, 0xe0);
+ addr_low = EREAD1(sc, 0xcd7);
+ EWRITE1(sc, 0xcd6, 0xe1);
+ addr_high = EREAD1(sc, 0xcd7);
+ addr = addr_high << 8 | addr_low;
+ EWRITE4(sc, addr, 0x30);
+ EWRITE4(sc, addr+0x04, 0x40);
+ EWRITE4(sc, addr, 0x34);
+ val = EREAD4(addr+0x04);
+ if (enable) {
+ val &= ~0x08;
+ val |= (1 << 4) | (1 << 9);
+ }
+ else {
+ val |= 0x08;
+ val &= ~((1 << 4) | (1 << 9));
+ }
+ EWRITE4(sc, addr+0x04, val);
+ }
+ else if (sc->amd_chipset.gen == AMD_CHIPSET_SB700 &&
+ sc->amd_chipset.rev <= 0x3b) {
+ addr = pci_conf_read(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xf0);
+ /* XXX These addresses are absolute, should be offsets? */
+ bus_space_write_4(sc->amd_chipset.nb_pa_iot, sc->amd_chipset.nb_io_handle,
+ addr, 0x30)
+ bus_space_write_4(sc->amd_chipset.nb_pa_iot, sc->amd_chipset.nb_io_handle,
+ addr+0x04, 0x40);
+ bus_space_write_4(sc->amd_chipset.nb_pa_iot, sc->amd_chipset.nb_io_handle,
+ addr, 0x34);
+ val = bus_space_read_4(sc->amd_chipset.nb_pa_iot, sc->amd_chipset.nb_io_handle,
+ addr+0x04);
+ if (enable) {
+ val &= ~0x08;
+ val |= (1 << 4) | (1 << 9);
+ }
+ else {
+ val |= 0x08;
+ val &= ~((1 << 4) | (1 << 9));
+ }
+ bus_space_write_4(sc->amd_chipset.nb_pa_iot, sc->amd_chipset.nb_io_handle,
+ addr+0x04, val);
+ }
+ else
+ return;
+
+ if (!sc->amd_chipset.nb_dev) {
+ return;
+ }
+
+ if (sc->amd_chipset.nb_type == 1 || sc->amd_chipset.nb_type == 3) {
+
+ /* XXX These addresses are absolute, should be offsets? */
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe0, 0x10040);
+ val = pci_conf_read(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4);
+
+ val &= ~(1 | (1 << 3) | (1 << 4) | (1 << 9) | (1 << 12));
+ val |= bit | (bit << 3) | (bit << 12);
+ val |= ((!bit) << 4) | ((!bit) << 9);
+
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4, val);
+
+
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe0, 0x10002);
+ val = pci_conf_read(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4);
+
+ val &= ~(1 << 8);
+ val |= bit << 8;
+
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4, val);
+ }
+ else if (sc->amd_chipset.nb_type == 2) {
+ /* XXX These addresses are absolute, should be offsets? */
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe0, 0x01100012);
+ val = pci_conf_read(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4);
+
+ if (disable)
+ val &= ~(0x3f << 7);
+ else
+ val |= 0x3f << 7;
+
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4, val);
+
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe0, 0x01100013);
+ val = pci_conf_read(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4);
+*/
+ if (disable)
+ val &= ~(0x3f << 7);
+ else
+ val |= 0x3f << 7;
+
+ pci_conf_write(sc->amd_chipset.nb_pa_pc, sc->amd_chipset.nb_pa_tag,
+ 0xe4, val);
+ }
}
Index: usb/ehci.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.200
diff -u -p -r1.200 ehci.c
--- usb/ehci.c 15 May 2017 10:52:08 -0000 1.200
+++ usb/ehci.c 6 Dec 2017 02:46:24 -0000
@@ -81,10 +81,11 @@ struct cfdriver ehci_cd = {
NULL, "ehci", DV_DULL
};
+#define EHCI_DEBUG
#ifdef EHCI_DEBUG
#define DPRINTF(x) do { if (ehcidebug) printf x; } while(0)
#define DPRINTFN(n,x) do { if (ehcidebug>(n)) printf x; } while (0)
-int ehcidebug = 0;
+int ehcidebug = 10;
#define bitmask_snprintf(q,f,b,l) snprintf((b), (l), "%b", (q), (f))
#else
#define DPRINTF(x)
@@ -3351,6 +3352,8 @@ ehci_device_isoc_start(struct usbd_xfer
TAILQ_INSERT_TAIL(&sc->sc_intrhead, ex, inext);
xfer->status = USBD_IN_PROGRESS;
xfer->done = 0;
+ if (sc->sc.sc_flags && EHCIF_AMD_PLL_QUIRK)
+ usb_amd_pll_quirk(sc, 1);
splx(s);
return (USBD_IN_PROGRESS);
@@ -3587,6 +3590,8 @@ ehci_device_isoc_done(struct usbd_xfer *
s = splusb();
epipe->u.isoc.cur_xfers--;
+ if (sc->sc.sc_flags && EHCIF_AMD_PLL_QUIRK)
+ usb_amd_pll_quirk(sc, 0);
if (xfer->status != USBD_NOMEM) {
ehci_rem_itd_chain(sc, ex);
ehci_free_itd_chain(sc, ex);
Index: usb/ehcivar.h
===================================================================
RCS file: /cvs/src/sys/dev/usb/ehcivar.h,v
retrieving revision 1.37
diff -u -p -r1.37 ehcivar.h
--- usb/ehcivar.h 2 Oct 2016 06:36:39 -0000 1.37
+++ usb/ehcivar.h 6 Dec 2017 02:46:24 -0000
@@ -121,6 +121,28 @@ struct ehci_soft_islot {
#define EHCI_FREE_LIST_INTERVAL 100
+/* Store info about amd chipset for workarounds */
+enum amd_chipset_gen {
+ NOT_AMD_CHIPSET = 0,
+ AMD_CHIPSET_SB600,
+ AMD_CHIPSET_SB700,
+ AMD_CHIPSET_SB800,
+ AMD_CHIPSET_HUDSON2,
+ AMD_CHIPSET_BOLTON,
+ AMD_CHIPSET_YANGTZE,
+ AMD_CHIPSET_UNKNOWN
+};
+struct amd_chipset_data {
+ int nb_type;
+ bus_space_tag_t nb_pa_iot;
+ bus_space_handle_t nb_io_handle;
+ pci_chipset_tag_t nb_pa_pc;
+ pcitag_t nb_pa_tag;
+ enum amd_chipset_gen gen;
+ int rev;
+ int isoc_count;
+};
+
struct ehci_softc {
struct usbd_bus sc_bus; /* base device */
bus_space_tag_t iot;
@@ -131,6 +153,8 @@ struct ehci_softc {
#define EHCIF_DROPPED_INTR_WORKAROUND 0x01
#define EHCIF_PCB_INTR 0x02
#define EHCIF_USBMODE 0x04
+#define EHCIF_AMD_PLL_QUIRK 0x08
+#define EHCIF_AMD_DUMMY_QH_QUIRK 0x10
char sc_vendor[16]; /* vendor string for root hub */
int sc_id_vendor; /* vendor ID for root hub */
@@ -164,6 +188,8 @@ struct ehci_softc {
struct rwlock sc_doorbell_lock;
struct timeout sc_tmo_intrlist;
+
+ struct amd_chipset_data amd_chipset;
};
#define EREAD1(sc, a) bus_space_read_1((sc)->iot, (sc)->ioh, (a))
Paul B. Henson
2017-12-07 22:46:42 UTC
Reply
Permalink
Raw Message
From: Martin Pieuchot
Sent: Thursday, December 7, 2017 3:18 AM
Which issue are you having?
Sorry, there was more context in an earlier thread. Basically, I have a pc engines APU3 board which has AMD Hudson-2 EHCI USB ports on it. If devices are plugged in when the system boots and the ports are initialized, the operating system sees they are there. However, if you hot plug a device after it is booted, or remove a device that was plugged in at boot, the system does not notice the change in state. Also, the Sierra wireless LTE modem I was trying to use does not function, the driver sends an open message over the USB bus to the device and then nothing ever comes back. Once the system is booted, the interrupt account for the ECHI ports from 'vmstat -I' never increases. The xHCI USB ports on the same system seemed to work fine, detecting hot plug/remove events, and properly initializing the wireless modem.
What makes you think that the quirks below
will help? What do you mean with 'work fine on those systems'? If they
work fine, which issues are you having?
The same board when booted up under either Linux or FreeBSD appears to have fully functional EHCI USB ports; they detect hot plug/remove events, and under linux the wireless modem is initialized and can successfully pass traffic (FreeBSD doesn't have a driver for it, so I was unable to test it there).

I honestly don't know if these specific quirks will resolve the issue under OpenBSD, all I know is that there is something Linux/FreeBSD is doing different regarding the USB hardware, and porting these quirks seemed a good place to start. I'm not really a low level hardware/device driver guy, so I'm flying a bit blind. Someone told me there are known issues with amd USB ports in general, such as ath based USB wireless cards not working, so these might help that problem even if it doesn't fix mine.
It depends how the controller is connected to the host. If you look at
the PCI glue driver, dev/pci/ehci_pci.c you'll see
115: /* Map I/O registers */
116: if (pci_mapreg_map(pa, PCI_CBMEM, PCI_MAPREG_TYPE_MEM, 0,
117: &sc->sc.iot, &sc->sc.ioh, NULL, &sc->sc.sc_size, 0))
Then in the EHCI driver, dev/usb/ehci.c, these are accessed via the
EREAD/EWRITE/EOREAD/EOWRITE macros.
Ah, ok; it appears that pci_mapreg_map defines the start of the region as:

ex = pa->pa_ioex;
if (ex != NULL) {
start = max(PCI_IO_START, ex->ex_start);

with:

#define PCI_IO_START 0

So the starting memory address is either 0 or pa->pa_ioex from the struct pci_attach_args that was passed into ehci_pci_attach.

Given the existing reads:

sc->sc.sc_offs = EREAD1(&sc->sc, EHCI_CAPLENGTH);
define EHCI_CAPLENGTH 0x00

I'm pretty sure it's not zero, so it must be the one from pa_ioex.
But maybe you just want to use pci_conf_read()/pci_conf_write()?
Hmm, given my lack of detailed knowledge of this area I can't say for sure. However, there are two different things being done in the linux code I am referring to:

outb_p(0xe0, 0xcd6);

This I believe writes the byte 0xe0 to the I/O port at address 0xcd6, where as this:

pci_write_config_dword(amd_chipset.nb_dev, 0xe4, val);

Writes the contents of the variable val to the PCI configuration register located at 0xe4. Those are two different operations, right?

So wherever the linux code writes to an absolute I/O port for the USB device, such as 0xcd6, I can subtract the beginning of the mapped region as stored in pa_ioex from it to arrive at the appropriate offset value to use with EREAD/WRITE?
Is low power mode enabled on OpenBSD?
Based on the comment in the linux code:

"The hardware normally enables the A-link power management feature, which
lets the system lower the power consumption in idle states."

I believe that is the default behavior of the hardware unless you explicitly do something otherwise with it.
The other quirk involves never having an empty frame list; I have
Post by Paul B. Henson
implemented the logic to detect when that is required, but haven't even
come close to wrapping my head around actually implementing the quirk
itself.
For which transfer type is this quirk required, isochronous only?
The explanation of this quirk is:

"EHCI controller on AMD SB700/SB800/Hudson-2/3 platforms may
read/write memory space which does not belong to it when
there is NULL pointer with T-bit set to 1 in the frame list
table. To avoid the issue, the frame list link pointer
should always contain a valid pointer to a inactive qh."

I am also unfortunately not that expert in the underlying USB hardware level protocol, but the quirk is referenced in two functions, scan_isoc:

if (!ehci->use_dummy_qh ||
q.itd->hw_next != EHCI_LIST_END(ehci))
*hw_p = q.itd->hw_next;
else
*hw_p = cpu_to_hc32(ehci, ehci->dummy->qh_dma);


and periodic_unlink:

if (!ehci->use_dummy_qh ||
*shadow_next_periodic(ehci, &here, Q_NEXT_TYPE(ehci, *hw_p))
!= EHCI_LIST_END(ehci))
*hw_p = *shadow_next_periodic(ehci, &here,
Q_NEXT_TYPE(ehci, *hw_p));
else
*hw_p = cpu_to_hc32(ehci, ehci->dummy->qh_dma);

Based solely on the name of the first function, it sounds like it might be.
time.
Okay, thanks; I wasn't sure what the etiquette was for posting to that list :).
Post by Paul B. Henson
+ /* AMD errata indicates 8111 chipset EHCI is broken */
+ if (PCI_PRODUCT(pa->pa_id) ==
PCI_PRODUCT_AMD_8111_EHCI) {
Post by Paul B. Henson
+ printf("%s: AMD 8111 EHCI broken, skipping",
devname);
Post by Paul B. Henson
+ goto disestablish_ret;
If it is broken we should not match it. That means ehci_pci_match()
should contain a check for this model an return 0 instead of 1.
Okay, I will move this check there.
This could be a diff on its own, but please make sure to give a
reference for where you found the piece of information. A vendor
document is always the best, does it exist an errata for this chip?
At least the Linux commit hash such that it makes reviewer life easy ;)
I'll try to track down a good reference for the information.
Post by Paul B. Henson
+ if (pci_find_device(&amd_pa, ehci_amd_pll_quirk_match)) {
+ sc->sc.amd_chipset.rev =
PCI_REVISION(amd_pa.pa_class);
Post by Paul B. Henson
+ if (PCI_PRODUCT(amd_pa.pa_id) ==
PCI_PRODUCT_ATI_SBX00_SMB) {
Post by Paul B. Henson
+ if (sc->sc.amd_chipset.rev >= 0x10 &&
+ sc->sc.amd_chipset.rev <= 0x1f)
+ sc->sc.amd_chipset.gen =
AMD_CHIPSET_SB600;
You don't need to reset the value, just read the code in this file, you
can use the PCI_REVISION() and PCI_PRODUCT() macro ;)
Hmm, I'm not sure I follow you? I am using the the PCI_REVISION() and PCI_PRODUCT() macros. The variable that is being set is actually a new one, the linux code does different stuff for a given USB controller depending on the revision of the northbridge chipset. This code reviews the combinations of PCI_PRODUCT and ranges of PCI_REVISION so it will know what to do later. Otherwise, every time it needed to bang the hardware to change the power mode it would have to figure it out again.

Thanks much for the info and advice, I will take another pass at the code and come back :).
Loading...