From: Mark Kettenis Subject: Re: return of the chromebook keyboard diff To: Miod Vallat Cc: tech@openbsd.org Date: Sun, 09 Feb 2025 21:48:08 +0100 > Date: Sun, 9 Feb 2025 19:27:19 +0000 > From: Miod Vallat > > > Thanks to off-list debugging with Miguel Landaeta, I have been able to > > improve these changes a lot. > > > > The previous iteration of this diff would not attach on his system > > because the keyboard controller is found before the GPIO controller. > > This is now fixed by postponing GPIO interrupt registration a bit. > > > > While I was working with interrupts, I have also reworked the attachment > > logic. > > It is now as follows: if your pckbc@acpi node has GPIO interrupts, or > > regular interrupts not configured in legacy, ISA-compatible (i.e. > > edge-triggered, rising front), then the legacy pckbc@isa has no chance > > to behave properly, and we attach; otherwise we let it claim the > > keyboard unless you change the device flags to 1 to force attachment. > > > > This should remove the need for a list of "known to need acpi > > attachment" devices, and in fact, I have removed it now. > > > > Please test. > > I have received a few positive reports (either working systems which are > not broken by this diff, or systems with a non-isa-compatible interrupt > configuration where this diff turns non-working keyboards into working > ones). But I'd like to get more feedback before this has a chance to get > in. The best way to collect such feedback is to get it in ;). > Unchanged diff below for convenience. A bit of feedback on the diff itself below. Thought I already provided it, but I must have been distracted the previous time you posted the diff and forgot about it. > Index: share/man/man4/acpi.4 > =================================================================== > RCS file: /OpenBSD/src/share/man/man4/acpi.4,v > diff -u -p -u -p -r1.76 acpi.4 > --- share/man/man4/acpi.4 5 Nov 2024 11:12:48 -0000 1.76 > +++ share/man/man4/acpi.4 31 Jan 2025 12:05:19 -0000 > @@ -132,6 +132,8 @@ Intel OnChip System Fabric device > Intelligent Platform Management Interface driver > .It Xr pchgpio 4 > Intel PCH GPIO controller > +.It Xr pckbc 4 > +Keyboard controller > .It Xr pluart 4 > ARM PrimeCell PL011 UART > .It Xr qcgpio 4 > Index: share/man/man4/pckbc.4 > =================================================================== > RCS file: /OpenBSD/src/share/man/man4/pckbc.4,v > diff -u -p -u -p -r1.18 pckbc.4 > --- share/man/man4/pckbc.4 26 Sep 2010 20:39:08 -0000 1.18 > +++ share/man/man4/pckbc.4 31 Jan 2025 12:05:19 -0000 > @@ -33,6 +33,7 @@ > .Nd PC (ISA) keyboard controller driver > .Sh SYNOPSIS > .Cd "pckbc* at isa? flags 0x00 " Pq "alpha, amd64, i386, loongson" > +.Cd "pckbc* at acpi? flags 0x00 " Pq "amd64" > .Cd "pckbc* at ebus? " Pq "sparc64" > .Cd "pckbd* at pckbc?" > .Cd "pms* at pckbc?" > @@ -40,21 +41,17 @@ > The > .Nm > driver handles resource allocation and device attachment for the > -traditional PC/AT keyboard controller. > -It provides two logical connections for child devices, the > +traditional PC/AT keyboard controller, or emulations thereof. > +It provides up to two logical connections for child devices, the > .Dq keyboard > slot for a keyboard and the > .Dq auxiliary > -slot for mice (the latter might be missing in older keyboard controllers). > -.\" .Pp > -.\" The optional > -.\" .Dq slot > -.\" locator argument can be used to force unusual connections of devices to > -.\" logical slots. > -.\" This feature is for experimentation only, it will not be > -.\" useful in normal operation. > +slot for mice (the latter might be missing in older keyboard controllers, > +or recent emulations). > .Pp > To avoid attaching a phantom PS/2 keyboard device, the > +.Xr isa 4 > +attachment of the > .Nm > driver will attempt to detect USB legacy keyboard emulation on amd64 and i386 > systems. > @@ -62,7 +59,18 @@ Unfortunately, the detection heuristics > PS/2 keyboard. > The keyboard can be forced to attach on these systems, by changing the > device flags to 1. > +.Pp > +The > +.Xr acpi 4 > +attachment of the > +.Nm > +driver defaults to attach only where it would perform better than its legacy > +.Xr isa 4 > +attachment. > +Should this logic be insufficient, it is possible to force it to always attach, > +by changing its device flags to 1. > .Sh SEE ALSO > +.Xr acpi 4 , > .Xr ebus 4 , > .Xr intro 4 , > .Xr isa 4 , > Index: sys/arch/amd64/conf/GENERIC > =================================================================== > RCS file: /OpenBSD/src/sys/arch/amd64/conf/GENERIC,v > diff -u -p -u -p -r1.530 GENERIC > --- sys/arch/amd64/conf/GENERIC 26 Nov 2024 21:45:35 -0000 1.530 > +++ sys/arch/amd64/conf/GENERIC 31 Jan 2025 12:05:19 -0000 > @@ -86,6 +86,7 @@ ipmi0 at acpi? disable > ccpmic* at iic? > tipmic* at iic? > intelpmc* at acpi? > +pckbc* at acpi? > > efi0 at bios0 > mpbios0 at bios0 > Index: sys/arch/amd64/conf/RAMDISK > =================================================================== > RCS file: /OpenBSD/src/sys/arch/amd64/conf/RAMDISK,v > diff -u -p -u -p -r1.87 RAMDISK > --- sys/arch/amd64/conf/RAMDISK 12 Aug 2024 18:43:41 -0000 1.87 > +++ sys/arch/amd64/conf/RAMDISK 31 Jan 2025 12:05:19 -0000 > @@ -37,6 +37,7 @@ acpimadt0 at acpi? > com0 at acpi? addr 0x3f8 > com1 at acpi? addr 0x2f8 > com* at acpi? > +pckbc* at acpi? > > mpbios0 at bios0 > > Index: sys/arch/amd64/conf/RAMDISK_CD > =================================================================== > RCS file: /OpenBSD/src/sys/arch/amd64/conf/RAMDISK_CD,v > diff -u -p -u -p -r1.209 RAMDISK_CD > --- sys/arch/amd64/conf/RAMDISK_CD 26 Nov 2024 21:45:35 -0000 1.209 > +++ sys/arch/amd64/conf/RAMDISK_CD 31 Jan 2025 12:05:19 -0000 > @@ -56,6 +56,7 @@ com1 at acpi? addr 0x2f8 > com2 at acpi? addr 0x3e8 > com* at acpi? > glkgpio* at acpi? > +pckbc* at acpi? > > mpbios0 at bios0 > > Index: sys/dev/acpi/acpi.c > =================================================================== > RCS file: /OpenBSD/src/sys/dev/acpi/acpi.c,v > diff -u -p -u -p -r1.440 acpi.c > --- sys/dev/acpi/acpi.c 23 Jan 2025 11:24:34 -0000 1.440 > +++ sys/dev/acpi/acpi.c 31 Jan 2025 12:05:19 -0000 > @@ -3037,12 +3037,9 @@ const char *acpi_skip_hids[] = { > > /* ISA devices for which we attach a driver later */ > const char *acpi_isa_hids[] = { > - "PNP0303", /* IBM Enhanced Keyboard (101/102-key, PS/2 Mouse) */ > "PNP0400", /* Standard LPT Parallel Port */ > "PNP0401", /* ECP Parallel Port */ > "PNP0700", /* PC-class Floppy Disk Controller */ > - "PNP0F03", /* Microsoft PS/2-style Mouse */ > - "PNP0F13", /* PS/2 Mouse */ > NULL > }; > > Index: sys/dev/acpi/files.acpi > =================================================================== > RCS file: /OpenBSD/src/sys/dev/acpi/files.acpi,v > diff -u -p -u -p -r1.71 files.acpi > --- sys/dev/acpi/files.acpi 4 Aug 2024 11:05:18 -0000 1.71 > +++ sys/dev/acpi/files.acpi 31 Jan 2025 12:05:19 -0000 > @@ -289,3 +289,7 @@ file dev/acpi/iosf_acpi.c iosf_acpi > device intelpmc > attach intelpmc at acpi > file dev/acpi/intelpmc.c intelpmc > + > +# PS/2 Keyboard Controller > +attach pckbc at acpi with pckbc_acpi > +file dev/acpi/pckbc_acpi.c pckbc_acpi > Index: sys/dev/acpi/pckbc_acpi.c > =================================================================== > RCS file: sys/dev/acpi/pckbc_acpi.c > diff -N sys/dev/acpi/pckbc_acpi.c > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ sys/dev/acpi/pckbc_acpi.c 31 Jan 2025 12:05:19 -0000 > @@ -0,0 +1,569 @@ > +/* $OpenBSD$ */ > +/* > + * Copyright (c) 2024, 2025, Miodrag Vallat. > + * > + * 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. > + */ > +/* > + * Copyright (c) 1998 > + * Matthias Drochner. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR > + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES > + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. > + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, > + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT > + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF > + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#include > +#include > +#include > +#include > + > +#include > + > +#include > +#include > +#include > +#include > + > +#include > +#include > + > +/* > + * This driver is more complicated than it should have to be, as it needs > + * to gather the needs of pckbc (2 I/O ports, and up to 2 interrupts), > + * which may be scattered across two ACPI nodes. > + * Because of this, it is able to attach to two different ACPI nodes, > + * but the second attachment "hijacks" the first one's softc struct, so > + * that all the required data is gathered in one place. > + */ > + > +int pckbc_acpi_match(struct device *, void *, void *); > +void pckbc_acpi_attach(struct device *, struct device *, void *); > +int pckbc_acpi_activate(struct device *, int); > + > +struct pckbc_acpi_gpio_intr { > + struct aml_node *node; > + uint16_t pin; > + uint16_t flags; > +}; > + > +struct pckbc_acpi_softc { > + struct pckbc_softc sc; > + /* regular interrupts */ > + void *sc_ih[2]; > + /* gpio interrupts */ > + struct pckbc_acpi_gpio_intr sc_gpioint[2]; > + unsigned int sc_nints, sc_ngpioints; > +}; > + > +const struct cfattach pckbc_acpi_ca = { > + .ca_devsize = sizeof(struct pckbc_acpi_softc), > + .ca_match = pckbc_acpi_match, > + .ca_attach = pckbc_acpi_attach, > + .ca_activate = pckbc_acpi_activate > +}; > + > +struct pckbc_acpi_crs_data { > + struct aml_node *basenode; > + struct pckbc_acpi_gpio_intr intrs[2]; > + unsigned int nints; > +}; > + > +int pckbc_acpi_match_kbd(struct device *, void *, void *); > +int pckbc_acpi_match_mou(struct device *, void *, void *, > + struct pckbc_acpi_softc *); > +void pckbc_acpi_attach_kbd(struct device *, struct device *, void *); > +void pckbc_acpi_attach_mou(struct device *, struct device *, void *); > +struct pckbc_acpi_softc *pckbc_acpi_find(int); > +void pckbc_acpi_crs_walk(struct device *, struct aml_node *, > + struct pckbc_acpi_crs_data *, > + int (*)(int, union acpi_resource *, void *)); > +int pckbc_acpi_getgpioirqcount(int, union acpi_resource *, void *); > +int pckbc_acpi_getgpioirqdata(int, union acpi_resource *, void *); > +void pckbc_acpi_register_gpio_intrs(struct device *); > +int pckbc_acpi_gpio_intr_wrapper(void *); > + > +int > +pckbc_acpi_match(struct device *parent, void *match, void *aux) > +{ > + struct pckbc_acpi_softc *sc = pckbc_acpi_find(1); > + if (sc == NULL) /* no pckbc@acpi attachment yet */ > + return pckbc_acpi_match_kbd(parent, match, aux); > + else > + return pckbc_acpi_match_mou(parent, match, aux, sc); > +} > + > +void > +pckbc_acpi_attach(struct device *parent, struct device *self, void *aux) > +{ > + struct pckbc_acpi_softc *sc = pckbc_acpi_find(2); > + if (sc == NULL) /* no second pckbc@acpi attachment yet */ > + return pckbc_acpi_attach_kbd(parent, self, aux); > + else > + return pckbc_acpi_attach_mou(parent, self, aux); > +} > + > +const char *pckbc_acpi_cids_kbd[] = { > + "PNP0303", /* IBM Enhanced Keyboard (101/102-key, PS/2 Mouse) */ > + NULL > +}; > + > +/* > + * Matching logic for the keyboard node. We want two I/O ports, at least > + * one interrupt, and either a HID match or a CID match (if explicitly > + * allowed, or if the interrupt is a GPIO interrupt). > + */ > +int > +pckbc_acpi_match_kbd(struct device *parent, void *match, void *aux) > +{ > + struct acpi_attach_args *aaa = aux; > + struct cfdata *cf = match; > + struct pckbc_acpi_crs_data crsdata; > + int irq, rv; > + > + if (aaa->aaa_naddr < 2) > + return 0; > + rv = acpi_matchhids(aaa, pckbc_acpi_cids_kbd, cf->cf_driver->cd_name); > + if (rv == 0) > + return 0; > + /* > + * If this device uses non-GPIO interrupts in an ISA-compatible > + * way (i.e. edge interrupt, active high), then do not attach > + * unless explicitly required by device flags. > + */ > + if (cf->cf_flags & 0x0001) > + return rv; > + if (aaa->aaa_nirq != 0) { > + for (irq = 0; irq < aaa->aaa_nirq; irq++) { > + if ((aaa->aaa_irq_flags[irq] & > + (LR_EXTIRQ_MODE | LR_EXTIRQ_POLARITY)) != > + LR_EXTIRQ_MODE) > + break; > + } > + if (irq == aaa->aaa_nirq) > + return 0; /* all legacy */ > + } else { > + pckbc_acpi_crs_walk(parent, aaa->aaa_node, &crsdata, > + pckbc_acpi_getgpioirqcount); > + if (crsdata.nints == 0) > + return 0; /* interrupt, where art thou? */ > + } > + > + if (rv == 0 && (aaa->aaa_nirq == 0 || (cf->cf_flags & 0x0001) != 0)) > + rv = acpi_matchhids(aaa, > + pckbc_acpi_cids_kbd + nitems(pckbc_acpi_cids_kbd) - 2, > + cf->cf_driver->cd_name); > + if (rv == 0) > + return 0; > + /* perform the expensive checks last */ > + if (aaa->aaa_nirq == 0) { > + pckbc_acpi_crs_walk(parent, aaa->aaa_node, &crsdata, > + pckbc_acpi_getgpioirqcount); > + if (crsdata.nints == 0) > + return 0; > + } > + return rv; > +} > + > +const char *pckbc_acpi_cids_mou[] = { I had to think twice what mou meant before I realized it is an abbreviation for mouse. So maybe it is a good idea to spell that out. We do after all have wskbd(4) but still use wsmouse(4). > + "PNP0F03", /* Microsoft PS/2-style Mouse */ > + "PNP0F13", /* PS/2 Mouse */ > + NULL > +}; > + > +/* > + * Matching logic for the mouse node. We want a previous keyboard to have > + * attached, and want an interrupt if the keyboard node didn't have two. > + */ > +int > +pckbc_acpi_match_mou(struct device *parent, void *match, void *aux, And same here of course. > + struct pckbc_acpi_softc *pasc) > +{ > + struct acpi_attach_args *aaa = aux; > + struct cfdata *cf = match; > + struct pckbc_acpi_crs_data crsdata; > + int rv; > + > + /* > + * We only need to attach the mouse node if the keyboard attachment > + * succeeded, we need interrupt information for the aux slot, and > + * this acpi node provides it. > + */ > + if ((pasc->sc_nints == 0 && pasc->sc_ngpioints == 0) || > + (pasc->sc_nints + pasc->sc_ngpioints == 2)) > + return 0; > + rv = acpi_matchhids(aaa, pckbc_acpi_cids_mou, cf->cf_driver->cd_name); > + if (rv == 0) > + return 0; > + /* perform the expensive checks last */ > + if (aaa->aaa_nirq == 0) { > + pckbc_acpi_crs_walk(parent, aaa->aaa_node, &crsdata, > + pckbc_acpi_getgpioirqcount); > + if (crsdata.nints == 0) > + return 0; > + } > + return rv; > +} > + > +int > +pckbc_acpi_activate(struct device *self, int act) > +{ > + struct pckbc_acpi_softc *pasc = (struct pckbc_acpi_softc *)self; > + struct pckbc_softc *sc = &pasc->sc; > + int rv = 0; > + > + switch (act) { > + case DVACT_SUSPEND: > + rv = config_activate_children(self, act); > + pckbc_stop(sc); > + break; > + case DVACT_RESUME: > + pckbc_reset(sc); > + rv = config_activate_children(self, act); > + break; > + default: > + rv = config_activate_children(self, act); > + break; > + } > + return rv; > +} > + > +void > +pckbc_acpi_attach_kbd(struct device *parent, struct device *self, void *aux) > +{ > + struct pckbc_acpi_softc *pasc = (struct pckbc_acpi_softc *)self; > + struct pckbc_softc *sc = &pasc->sc; > + struct acpi_attach_args *aaa = aux; > + struct pckbc_internal *t; > + struct pckbc_acpi_crs_data crsdata; > + bus_space_handle_t ioh_d, ioh_c; > + int irq, rv; > + > + if (aaa->aaa_nirq == 0) > + pckbc_acpi_crs_walk(parent, aaa->aaa_node, &crsdata, > + pckbc_acpi_getgpioirqdata); > + > + printf(" addr 0x%llx/0x%llx 0x%llx/0x%llx", aaa->aaa_addr[0], > + aaa->aaa_size[0], aaa->aaa_addr[1], aaa->aaa_size[1]); > + if (aaa->aaa_nirq == 0) { > + printf(" gpio irq pin"); > + for (irq = 0; irq < crsdata.nints && > + irq < nitems(pasc->sc_gpioint); irq++) > + printf(" %d", crsdata.intrs[irq].pin); > + } else { > + printf(" irq"); > + for (irq = 0; irq < aaa->aaa_nirq && irq < nitems(pasc->sc_ih); > + irq++) > + printf(" %d", aaa->aaa_irq[irq]); > + } > + printf(": \"%s\"\n", aaa->aaa_dev); > + > + if (aaa->aaa_nirq == 0) { > + /* Remember GPIO interrupt details for later setup */ > + for (irq = 0; irq < crsdata.nints; irq++) { > + if (pasc->sc_ngpioints == nitems(pasc->sc_gpioint)) > + break; > + pasc->sc_gpioint[pasc->sc_ngpioints++] = > + crsdata.intrs[irq]; /* struct copy */ > + } > + } else { > + for (irq = 0; irq < aaa->aaa_nirq; irq++) { > + if (pasc->sc_nints == nitems(pasc->sc_ih)) > + break; > + pasc->sc_ih[pasc->sc_nints] = acpi_intr_establish( > + aaa->aaa_irq[irq], aaa->aaa_irq_flags[irq], IPL_TTY, > + pckbcintr, pasc, self->dv_xname); > + if (pasc->sc_ih[pasc->sc_nints] == NULL) { > + printf("%s: can't establish interrupt %d\n", > + self->dv_xname, aaa->aaa_irq[irq]); > + goto fail_intr; > + } > + pasc->sc_nints++; > + } > + } > + > + if (pckbc_is_console(aaa->aaa_bst[0], aaa->aaa_addr[0])) { > + t = &pckbc_consdata; > + pckbc_console_attached = 1; > + /* t->t_cmdbyte was initialized by cnattach */ > + } else { > + if ((rv = bus_space_map(aaa->aaa_bst[0], aaa->aaa_addr[0], 1, 0, > + &ioh_d)) != 0) { > + printf("%s: couldn't map data port (%d)\n", > + self->dv_xname, rv); > + goto fail_mapd; > + } > + if ((rv = bus_space_map(aaa->aaa_bst[1], aaa->aaa_addr[1], 1, 0, > + &ioh_c)) != 0) { > + printf("%s: couldn't map command port (%d)\n", > + self->dv_xname, rv); > + goto fail_mapc; > + } > + > + t = malloc(sizeof(*t), M_DEVBUF, M_WAITOK | M_ZERO); > + /* > + * pckbc should theoretically be updated to use separate > + * bus_space_tag_t for the data and command ports, since on > + * this particular attachment they appear as separate I/O > + * resources. But since these are I/O resources, all > + * aaa_bst[] are identical, so we can avoid this change > + * for the time being as long as the logic in > + * acpi_parse_resources() does not change. > + */ > + t->t_iot = aaa->aaa_bst[0]; > + t->t_ioh_d = ioh_d; > + t->t_ioh_c = ioh_c; > + t->t_cmdbyte = KC8_CPU; /* Enable ports */ > + } > + > + t->t_sc = sc; > + sc->id = t; > + > + /* > + * Make sure pckbc@isa will not try to attach. > + */ > + { > + extern int pckbc_acpi; > + pckbc_acpi = 1; > + } > + > + pckbc_attach(sc, 0); > + config_defer(self, pckbc_acpi_register_gpio_intrs); > + return; > + > + fail_mapc: > + bus_space_unmap(aaa->aaa_bst[0], ioh_d, 1); > + fail_mapd: > + fail_intr: > + if (aaa->aaa_nirq != 0) { > + for (irq = pasc->sc_nints - 1; irq >= 0; irq--) > + acpi_intr_disestablish(pasc->sc_ih[irq]); > + pasc->sc_nints = 0; > + } > +} > + > +void > +pckbc_acpi_attach_mou(struct device *parent, struct device *self, void *aux) > +{ > + struct pckbc_acpi_softc *pasc = pckbc_acpi_find(1); > + struct acpi_attach_args *aaa = aux; > + struct pckbc_acpi_crs_data crsdata; > + int irq, base; > + > + if (aaa->aaa_nirq == 0) > + pckbc_acpi_crs_walk(parent, aaa->aaa_node, &crsdata, > + pckbc_acpi_getgpioirqdata); > + > + if (aaa->aaa_nirq == 0) { > + printf(" gpio irq pin"); > + for (irq = 0; irq < crsdata.nints && > + irq < nitems(pasc->sc_gpioint) - pasc->sc_ngpioints; irq++) > + printf(" %d", crsdata.intrs[irq].pin); > + } else { > + printf(" irq"); > + for (irq = 0; irq < aaa->aaa_nirq && > + irq < nitems(pasc->sc_ih) - pasc->sc_nints; irq++) > + printf(" %d", aaa->aaa_irq[irq]); > + } > + printf(": \"%s\"\n", aaa->aaa_dev); > + > + if (aaa->aaa_nirq == 0) { > + /* Remember GPIO interrupt details for later setup */ > + for (irq = 0; irq < crsdata.nints; irq++) { > + if (pasc->sc_ngpioints == nitems(pasc->sc_gpioint)) > + break; > + pasc->sc_gpioint[pasc->sc_ngpioints++] = > + crsdata.intrs[irq]; /* struct copy */ > + } > + } else { > + base = pasc->sc_nints; > + for (irq = 0; irq < aaa->aaa_nirq; irq++) { > + if (pasc->sc_nints == nitems(pasc->sc_ih)) > + break; > + pasc->sc_ih[pasc->sc_nints] = acpi_intr_establish( > + aaa->aaa_irq[irq], aaa->aaa_irq_flags[irq], IPL_TTY, > + pckbcintr, pasc, self->dv_xname); > + if (pasc->sc_ih[pasc->sc_nints] == NULL) { > + printf("%s: can't establish interrupt %d\n", > + self->dv_xname, aaa->aaa_irq[irq]); > + goto fail_intr; > + } > + pasc->sc_nints++; > + } > + } > + > + return; > + > + fail_intr: > + if (aaa->aaa_nirq != 0) { > + for (irq = pasc->sc_nints - 1; irq >= base; irq--) > + acpi_intr_disestablish(pasc->sc_ih[irq]); > + pasc->sc_nints = base; > + } > +} > + > +/* > + * Register all GPIO interrupts. > + * This is done after all acpi devices have attached, as the GPIO handler > + * may attach after us. > + */ > +void > +pckbc_acpi_register_gpio_intrs(struct device *dev) > +{ > + struct pckbc_acpi_softc *sc = (struct pckbc_acpi_softc *)dev; > + int irq; > + > + for (irq = 0; irq < sc->sc_ngpioints; irq++) { > + struct acpi_gpio *gpio = sc->sc_gpioint[irq].node->gpio; > + if (gpio == NULL) { > + printf("%s: unable to setup gpio pin %d interrupt\n", > + dev->dv_xname, sc->sc_gpioint[irq].pin); > + continue; > + } > + gpio->intr_establish(gpio->cookie, > + sc->sc_gpioint[irq].pin, sc->sc_gpioint[irq].flags, > + pckbc_acpi_gpio_intr_wrapper, sc); > + } > +} > + > +/* > + * Iterate over all pckbc attachments. The `nth' argument tells us which > + * pckbc@acpi device to return. > + * > + * Note that, at `match' time, the device we may end up attaching is not > + * found, but at `attach' time, it will be found. > + */ > +struct pckbc_acpi_softc * > +pckbc_acpi_find(int nth) > +{ > + extern struct cfdriver pckbc_cd; > + struct device *sc; > + int devno; > + > + for (devno = 0; devno < pckbc_cd.cd_ndevs; devno++) { > + if ((sc = pckbc_cd.cd_devs[devno]) == NULL) > + continue; > + if (sc->dv_cfdata->cf_attach != &pckbc_acpi_ca) > + continue; > + if (--nth == 0) > + return (struct pckbc_acpi_softc *)sc; > + } > + > + return NULL; > +} > + > +/* > + * _CRS resource walker. > + */ > +void > +pckbc_acpi_crs_walk(struct device *acpidev, struct aml_node *basenode, > + struct pckbc_acpi_crs_data *crsdata, > + int (*walker)(int, union acpi_resource *, void *)) > +{ > + struct aml_value val; > + > + memset(crsdata, 0, sizeof *crsdata); > + crsdata->basenode = basenode; > + if (aml_evalname((struct acpi_softc *)acpidev, basenode, "_CRS", > + 0, NULL, &val) != 0) > + return; > + if (val.type == AML_OBJTYPE_BUFFER && val.length >= 5) > + aml_parse_resource(&val, walker, crsdata); > + aml_freevalue(&val); > +} > + > +/* > + * _CRS walker callback, which counts GPIO interrupts. > + */ > +int > +pckbc_acpi_getgpioirqcount(int crsidx, union acpi_resource *crs, void *arg) > +{ > + struct pckbc_acpi_crs_data *crsdata = arg; > + struct aml_node *node; > + > + if (crsdata->nints == nitems(crsdata->intrs)) > + return 0; > + > + switch (AML_CRSTYPE(crs)) { > + case LR_GPIO: > + if (crs->lr_gpio.type != LR_GPIO_INT) > + break; > + node = aml_searchname(crsdata->basenode, > + (char *)&crs->pad[crs->lr_gpio.res_off]); > + if (node != NULL) > + crsdata->nints++; > + break; > + } > + return 0; > +} > + > +/* > + * _CRS walker callback, which registers GPIO interrupt details. > + */ > +int > +pckbc_acpi_getgpioirqdata(int crsidx, union acpi_resource *crs, void *arg) > +{ > + struct pckbc_acpi_crs_data *crsdata = arg; > + struct aml_node *node; > + > + if (crsdata->nints == nitems(crsdata->intrs)) > + return 0; > + > + switch (AML_CRSTYPE(crs)) { > + case LR_GPIO: > + if (crs->lr_gpio.type != LR_GPIO_INT) > + break; > + node = aml_searchname(crsdata->basenode, > + (char *)&crs->pad[crs->lr_gpio.res_off]); > + if (node != NULL) { > + crsdata->intrs[crsdata->nints].node = node; > + crsdata->intrs[crsdata->nints].pin = > + *(uint16_t *)&crs->pad[crs->lr_gpio.pin_off]; > + crsdata->intrs[crsdata->nints].flags = > + crs->lr_gpio.tflags; > + crsdata->nints++; > + } > + break; > + } > + return 0; > +} > + > +/* > + * Wrapper for GPIO interrupts, to enforce IPL_TTY. > + */ > +int > +pckbc_acpi_gpio_intr_wrapper(void *arg) > +{ > + int s, rv; > + > + s = spltty(); > + rv = pckbcintr(arg); > + splx(s); > + return rv; > +} > Index: sys/dev/ic/pckbc.c > =================================================================== > RCS file: /OpenBSD/src/sys/dev/ic/pckbc.c,v > diff -u -p -u -p -r1.55 pckbc.c > --- sys/dev/ic/pckbc.c 26 Aug 2023 15:01:00 -0000 1.55 > +++ sys/dev/ic/pckbc.c 31 Jan 2025 12:05:19 -0000 > @@ -89,6 +89,10 @@ int pckbc_console_attached; > int pckbc_console; > static struct pckbc_slotdata pckbc_cons_slotdata; > > +#ifdef __HAVE_ACPI > +int pckbc_acpi; > +#endif > + > static int pckbc_wait_output(bus_space_tag_t, bus_space_handle_t); > > static int pckbc_get8042cmd(struct pckbc_internal *); > Index: sys/dev/isa/pckbc_isa.c > =================================================================== > RCS file: /OpenBSD/src/sys/dev/isa/pckbc_isa.c,v > diff -u -p -u -p -r1.19 pckbc_isa.c > --- sys/dev/isa/pckbc_isa.c 18 Aug 2015 06:54:00 -0000 1.19 > +++ sys/dev/isa/pckbc_isa.c 31 Jan 2025 12:05:19 -0000 > @@ -56,6 +56,15 @@ pckbc_isa_match(struct device *parent, v > bus_space_handle_t ioh_d, ioh_c; > int res; > > +#ifdef __HAVE_ACPI > + /* Don't try anything if pckbc@acpi has claimed the keyboard. */ > + { > + extern int pckbc_acpi; > + if (pckbc_acpi) > + return 0; > + } > +#endif Do we really need this check? On the isa(4) bus we typically check whether we can map the bus address in the match function and return 0 if that fails. And mapping should fail if the address is already in use. So if we sucessfully attached pckbc@acpi, attaching pckbc@isa should already fail. And if we're dealing with the console keyboard here, then pckbc@acpi should already have set pckbc_console_attach and therefore pckbc_is_console() will return 0 so we'd stull call bus_space_map() which will fail. > + > /* If values are hardwired to something that they can't be, punt. */ > if ((ia->ia_iobase != IOBASEUNK && ia->ia_iobase != IO_KBD) || > ia->ia_maddr != MADDRUNK || > >