On 05/12/17(Tue) 18:48, Paul B. Henson wrote:
> 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?
> 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()?
> 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?
> 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?
> 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.
> 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 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 ;)
> + 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 ;)
> + }
> + 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))
>
