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))

Reply via email to