On Wed, Dec 29, 2021 at 06:02:02PM +0100, Mark Kettenis wrote:
> > Date: Wed, 29 Dec 2021 17:16:36 +0100
> > From: Patrick Wildt <[email protected]>
> >
> > On Wed, Dec 29, 2021 at 05:03:50PM +0100, Patrick Wildt wrote:
> > > Hi,
> > >
> > > a device I have still has oHCI and eHCI controllers, which are needed to
> > > be able to use the second USB port.
> > >
> > > ehci(4) at acpi(4) is plain simple, as it attaches to PNP0D20. I have
> > > copied the xhci(4) at acpi(4) file and added the functional changes that
> > > exist between xhci(4) and ehci(4) at fdt(4).
> > >
> > > ohci(4) is very similar, I have also added the functional changes that
> > > way. There's a difference in how they attach: It uses a PRP0001 ID,
> > > which basically is a device-tree namespace link. That means we match
> > > on that link *and* have to check the 'compatible' property.
> > >
> > > With these changes, my second USB port works just fine.
> > >
> > > Patrick
> >
> > Actually, there's a better way to match ohci(4). We can match on _CLS,
> > like we do for ahci(4) at acpi(4). Updated diff below.
>
> Yes, that is cleaner...
>
> > diff --git a/sys/arch/arm64/conf/GENERIC b/sys/arch/arm64/conf/GENERIC
> > index 19fd6e03c4f..879d024503f 100644
> > --- a/sys/arch/arm64/conf/GENERIC
> > +++ b/sys/arch/arm64/conf/GENERIC
> > @@ -54,7 +54,9 @@ com* at acpi?
> > dwgpio* at acpi?
> > dwiic* at acpi?
> > iic* at dwiic?
> > +ehci* at acpi?
> > ipmi* at acpi?
> > +ohci* at acpi?
> > pluart* at acpi?
> > sdhc* at acpi?
> > xhci* at acpi?
> > diff --git a/sys/arch/arm64/conf/RAMDISK b/sys/arch/arm64/conf/RAMDISK
> > index b505e37b399..caf2eea44b8 100644
> > --- a/sys/arch/arm64/conf/RAMDISK
> > +++ b/sys/arch/arm64/conf/RAMDISK
> > @@ -44,6 +44,8 @@ pci* at acpipci?
> > ahci* at acpi?
> > ccp* at acpi? # AMD Cryptographic Co-processor
> > com* at acpi?
> > +ehci* at acpi?
> > +ohci* at acpi?
> > pluart* at acpi?
> > sdhc* at acpi?
> > xhci* at acpi?
> > diff --git a/sys/dev/acpi/ehci_acpi.c b/sys/dev/acpi/ehci_acpi.c
> > new file mode 100644
> > index 00000000000..cd1777f26e4
> > --- /dev/null
> > +++ b/sys/dev/acpi/ehci_acpi.c
> > @@ -0,0 +1,125 @@
> > +/* $OpenBSD: ehci_acpi.c,v 1.4 2021/12/21 20:53:46 kettenis Exp $ */
> > +/*
> > + * Copyright (c) 2018 Mark Kettenis
> > + *
> > + * Permission to use, copy, modify, and distribute this software for any
> > + * purpose with or without fee is hereby granted, provided that the above
> > + * copyright notice and this permission notice appear in all copies.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > + */
> > +
> > +#include <sys/param.h>
> > +#include <sys/systm.h>
> > +#include <sys/device.h>
> > +
> > +#include <machine/bus.h>
> > +#include <machine/intr.h>
> > +
> > +#include <dev/acpi/acpireg.h>
> > +#include <dev/acpi/acpivar.h>
> > +#include <dev/acpi/acpidev.h>
> > +#include <dev/acpi/amltypes.h>
> > +#include <dev/acpi/dsdt.h>
> > +
> > +#include <dev/usb/usb.h>
> > +#include <dev/usb/usbdi.h>
> > +#include <dev/usb/usbdivar.h>
> > +#include <dev/usb/usb_mem.h>
> > +
> > +#include <dev/usb/ehcireg.h>
> > +#include <dev/usb/ehcivar.h>
> > +
> > +struct ehci_acpi_softc {
> > + struct ehci_softc sc;
> > + struct acpi_softc *sc_acpi;
> > + struct aml_node *sc_node;
> > + void *sc_ih;
> > +};
> > +
> > +int ehci_acpi_match(struct device *, void *, void *);
> > +void ehci_acpi_attach(struct device *, struct device *, void *);
> > +
> > +struct cfattach ehci_acpi_ca = {
> > + sizeof(struct ehci_acpi_softc), ehci_acpi_match, ehci_acpi_attach
> > +};
> > +
> > +const char *ehci_hids[] = {
> > + "PNP0D20",
> > + NULL
> > +};
> > +
> > +int
> > +ehci_acpi_match(struct device *parent, void *match, void *aux)
> > +{
> > + struct acpi_attach_args *aaa = aux;
> > + struct cfdata *cf = match;
> > +
> > + if (aaa->aaa_naddr < 1 || aaa->aaa_nirq < 1)
> > + return 0;
> > + return acpi_matchhids(aaa, ehci_hids, cf->cf_driver->cd_name);
> > +}
> > +
> > +void
> > +ehci_acpi_attach(struct device *parent, struct device *self, void *aux)
> > +{
> > + struct ehci_acpi_softc *sc = (struct ehci_acpi_softc *)self;
> > + struct acpi_attach_args *aaa = aux;
> > + int error;
> > +
> > + sc->sc_acpi = (struct acpi_softc *)parent;
> > + sc->sc_node = aaa->aaa_node;
> > + printf(" %s", sc->sc_node->name);
> > +
> > + printf(" addr 0x%llx/0x%llx", aaa->aaa_addr[0], aaa->aaa_size[0]);
> > + printf(" irq %d", aaa->aaa_irq[0]);
> > +
> > + sc->sc.iot = aaa->aaa_bst[0];
> > + sc->sc.sc_size = aaa->aaa_size[0];
> > + sc->sc.sc_bus.dmatag = aaa->aaa_dmat;
> > +
> > + if (bus_space_map(sc->sc.iot, aaa->aaa_addr[0], aaa->aaa_size[0],
> > + 0, &sc->sc.ioh)) {
> > + printf(": can't map registers\n");
> > + return;
> > + }
> > +
> > + /* Disable interrupts, so we don't get any spurious ones. */
> > + sc->sc.sc_offs = EREAD1(&sc->sc, EHCI_CAPLENGTH);
> > + EOWRITE2(&sc->sc, EHCI_USBINTR, 0);
> > +
> > + sc->sc_ih = acpi_intr_establish(aaa->aaa_irq[0], aaa->aaa_irq_flags[0],
> > + IPL_USB, ehci_intr, sc, sc->sc.sc_bus.bdev.dv_xname);
> > + if (sc->sc_ih == NULL) {
> > + printf(": can't establish interrupt\n");
> > + goto unmap;
> > + }
> > +
> > + printf("\n");
> > +
> > + strlcpy(sc->sc.sc_vendor, "Generic", sizeof(sc->sc.sc_vendor));
> > + if ((error = ehci_init(&sc->sc)) != 0) {
> > + printf("%s: init failed, error=%d\n",
> > + sc->sc.sc_bus.bdev.dv_xname, error);
> > + goto disestablish_ret;
> > + }
> > +
> > + /* Attach usb device. */
> > + config_found(self, &sc->sc.sc_bus, usbctlprint);
> > +
> > + return;
> > +
> > +disestablish_ret:
> > +#ifdef notyet
> > + acpi_intr_disestablish(sc->sc_ih);
>
> We have acpi_intr_disestablish() on arm64 now...
>
> > +#endif
> > +unmap:
> > + bus_space_unmap(sc->sc.iot, sc->sc.ioh, sc->sc.sc_size);
> > + return;
> > +}
> > diff --git a/sys/dev/acpi/files.acpi b/sys/dev/acpi/files.acpi
> > index 95df4ffe448..da8e38d8a79 100644
> > --- a/sys/dev/acpi/files.acpi
> > +++ b/sys/dev/acpi/files.acpi
> > @@ -187,6 +187,14 @@ file dev/acpi/pluart_acpi.c pluart_acpi
> > attach sdhc at acpi with sdhc_acpi
> > file dev/acpi/sdhc_acpi.c sdhc_acpi
> >
> > +# OHCI
> > +attach ohci at acpi with ohci_acpi
> > +file dev/acpi/ohci_acpi.c ohci_acpi
> > +
> > +# EHCI
> > +attach ehci at acpi with ehci_acpi
> > +file dev/acpi/ehci_acpi.c ehci_acpi
> > +
> > # XHCI
> > attach xhci at acpi with xhci_acpi
> > file dev/acpi/xhci_acpi.c xhci_acpi
> > diff --git a/sys/dev/acpi/ohci_acpi.c b/sys/dev/acpi/ohci_acpi.c
> > new file mode 100644
> > index 00000000000..f596305378a
> > --- /dev/null
> > +++ b/sys/dev/acpi/ohci_acpi.c
> > @@ -0,0 +1,172 @@
> > +/* $OpenBSD: ohci_acpi.c,v 1.4 2021/12/21 20:53:46 kettenis Exp $ */
> > +/*
> > + * Copyright (c) 2018 Mark Kettenis
> > + *
> > + * Permission to use, copy, modify, and distribute this software for any
> > + * purpose with or without fee is hereby granted, provided that the above
> > + * copyright notice and this permission notice appear in all copies.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > + */
> > +
> > +#include <sys/param.h>
> > +#include <sys/systm.h>
> > +#include <sys/device.h>
> > +
> > +#include <machine/bus.h>
> > +#include <machine/intr.h>
> > +
> > +#include <dev/acpi/acpireg.h>
> > +#include <dev/acpi/acpivar.h>
> > +#include <dev/acpi/acpidev.h>
> > +#include <dev/acpi/amltypes.h>
> > +#include <dev/acpi/dsdt.h>
> > +
> > +#include <dev/usb/usb.h>
> > +#include <dev/usb/usbdi.h>
> > +#include <dev/usb/usbdivar.h>
> > +#include <dev/usb/usb_mem.h>
> > +
> > +#include <dev/usb/ohcireg.h>
> > +#include <dev/usb/ohcivar.h>
> > +
> > +struct ohci_acpi_softc {
> > + struct ohci_softc sc;
> > + struct acpi_softc *sc_acpi;
> > + struct aml_node *sc_node;
> > + void *sc_ih;
> > +};
> > +
> > +int ohci_acpi_match(struct device *, void *, void *);
> > +void ohci_acpi_attach(struct device *, struct device *, void *);
> > +
> > +struct cfattach ohci_acpi_ca = {
> > + sizeof(struct ohci_acpi_softc), ohci_acpi_match, ohci_acpi_attach
> > +};
> > +
> > +void ohci_acpi_attach_deferred(struct device *);
> > +
> > +int
> > +ohci_acpi_match(struct device *parent, void *match, void *aux)
> > +{
> > + struct acpi_attach_args *aaa = aux;
> > +
> > + return acpi_matchcls(aaa, PCI_CLASS_SERIALBUS,
> > + PCI_SUBCLASS_SERIALBUS_USB, PCI_INTERFACE_OHCI);
> > +}
> > +
> > +void
> > +ohci_acpi_attach(struct device *parent, struct device *self, void *aux)
> > +{
> > + struct ohci_acpi_softc *sc = (struct ohci_acpi_softc *)self;
> > + struct acpi_attach_args *aaa = aux;
> > + int error;
> > +
> > + sc->sc_acpi = (struct acpi_softc *)parent;
> > + sc->sc_node = aaa->aaa_node;
> > + printf(" %s", sc->sc_node->name);
> > +
> > + printf(" addr 0x%llx/0x%llx", aaa->aaa_addr[0], aaa->aaa_size[0]);
> > + printf(" irq %d", aaa->aaa_irq[0]);
> > +
> > + sc->sc.iot = aaa->aaa_bst[0];
> > + sc->sc.sc_size = aaa->aaa_size[0];
> > + sc->sc.sc_bus.dmatag = aaa->aaa_dmat;
> > +
> > + if (bus_space_map(sc->sc.iot, aaa->aaa_addr[0], aaa->aaa_size[0],
> > + 0, &sc->sc.ioh)) {
> > + printf(": can't map registers\n");
> > + return;
> > + }
> > +
> > + /* Record what interrupts were enabled by SMM/BIOS. */
> > + sc->sc.sc_intre = bus_space_read_4(sc->sc.iot, sc->sc.ioh,
> > + OHCI_INTERRUPT_ENABLE);
> > +
> > + /* Disable interrupts, so we don't get any spurious ones. */
> > + bus_space_write_4(sc->sc.iot, sc->sc.ioh, OHCI_INTERRUPT_DISABLE,
> > + OHCI_MIE);
> > +
> > + bus_space_barrier(sc->sc.iot, sc->sc.ioh, 0, sc->sc.sc_size,
> > + BUS_SPACE_BARRIER_READ|BUS_SPACE_BARRIER_WRITE);
> > + bus_space_write_4(sc->sc.iot, sc->sc.ioh,
> > + OHCI_INTERRUPT_DISABLE, OHCI_MIE);
> > +
> > + /* Map and establish the interrupt. */
> > + splassert(IPL_USB);
>
> I don't think that splassert is very useful.
The pci version does an splusb() and splx() dance. Should we keep that
in sync, or just remove all that spl stuff in this version?
> > + sc->sc_ih = acpi_intr_establish(aaa->aaa_irq[0], aaa->aaa_irq_flags[0],
> > + IPL_USB, ohci_intr, sc, sc->sc.sc_bus.bdev.dv_xname);
> > + if (sc->sc_ih == NULL) {
> > + printf(": can't establish interrupt\n");
> > + goto unmap;
> > + }
> > +
> > + printf(": ");
> > +
> > + strlcpy(sc->sc.sc_vendor, "Generic", sizeof(sc->sc.sc_vendor));
> > +
> > + /* Display revision. Legacy handover isn't needed as there's no smm */
>
> That is not necessarily true with ACPI. But I suppose as long as we
> only enable this on arm64...
You're right, maybe I should just use the pci version here and call
ohci_handover(). It probably doesn't hurt, I'll give it a try.
> > + if (ohci_checkrev(&sc->sc) != USBD_NORMAL_COMPLETION) {
> > + goto disestablish_ret;
> > + }
>
> Don't need those curly braces.
Will fix.
>
> > +
> > + /* Ignore interrupts for now */
> > + sc->sc.sc_bus.dying = 1;
> > +
> > + config_defer(self, ohci_acpi_attach_deferred);
> > +
> > + if ((error = ohci_init(&sc->sc)) != 0) {
>
> I hate assignments in if statements.
Ok.
>
> > + printf("%s: init failed, error=%d\n",
> > + sc->sc.sc_bus.bdev.dv_xname, error);
> > + goto disestablish_ret;
> > + }
> > +
> > + /* Attach usb device. */
> > + config_found(self, &sc->sc.sc_bus, usbctlprint);
> > +
> > + return;
> > +
> > +disestablish_ret:
> > +#ifdef notyet
> > + acpi_intr_disestablish(sc->sc_ih);
>
> We have acpi_intr_disestablish() on arm64 now...
>
> > +#endif
> > +unmap:
> > + bus_space_unmap(sc->sc.iot, sc->sc.ioh, sc->sc.sc_size);
> > + return;
> > +}
> > +
> > +void
> > +ohci_acpi_attach_deferred(struct device *self)
> > +{
> > + struct ohci_acpi_softc *sc = (struct ohci_acpi_softc *)self;
> > + usbd_status r;
> > + int s;
> > +
> > + s = splusb();
> > +
> > + sc->sc.sc_bus.dying = 0;
> > +
> > + r = ohci_init(&sc->sc);
> > +
> > + splx(s);
>
> Formatting is a bit weird here. I would remove the blank lines above.
>
> > + if (r != USBD_NORMAL_COMPLETION) {
> > + printf("%s: init failed, error=%d\n",
> > + sc->sc.sc_bus.bdev.dv_xname, r);
> > +#ifdef notyet
> > + acpi_intr_disestablish(sc->sc_ih);
> > +#endif
> > + sc->sc_ih = NULL;
> > + bus_space_unmap(sc->sc.iot, sc->sc.ioh, sc->sc.sc_size);
> > + sc->sc.sc_size = 0;
> > + return;
> > + }
> > +
> > + /* Attach usb device. */
> > + config_found(self, &sc->sc.sc_bus, usbctlprint);
> > +}
> >
> >
