On Wed, Apr 20, 2022 at 11:39:00AM +0200, Mark Kettenis wrote:
> > Date: Tue, 19 Apr 2022 22:02:00 -0700
> > From: Mike Larkin <mlar...@nested.page>
> >
> > On at least the Asus ROG Zephyrus 14 (2020), the trackpad fails to generate
> > any interrupts after resume. I tracked this down to amdgpio(4) not
> > generating
> > interrupts after resume, and started looking at missing soft state.
> >
> > This diff preserves the interrupt pin configurations and restores them after
> > resume. This makes the device function properly post-zzz and post-ZZZ.
>
> I think it might make sense to structure this a bit more like
> pchgpio(4). There we only restore the configuration for pins that are
> "in use" by OpenBSD.
>
> > Note: amdgpio_read_pin does not return the value that was previously written
> > during amdgpio_intr_establish (it always just returns 0x1 if the pin is
> > in use), so I'm just saving the actual value we write during
> > amdgpio_intr_establish and restoring that during resume.
>
> Well, using amdgpio_read_pin() for the purpose of saving the pin
> configuration doesn't make sense. That function returns the pin input
> state.
>
> What you need to do is to read the register using bus_space_read_4()
> and restore that value. Again take a look at pchgpio(4).
>
> > Note 2: In xxx_activate() functions, we usually call
> > config_activate_children
> > but since amdgpio doesn't have any children, I left that out.
>
> I think that's fine. But you should do the save/restore in
> DVACT_SUSPEND/DVACT_RESUME. You want to restore the state as early as
> possible such that you don't get spurious interrupts when the BIOS
> leaves GPIO pins misconfigured. Again, look at pchgpio(4).
>
Will take a look, thanks!
-ml
> >
> > ok?
> >
> > -ml
> >
> >
> > diff a82721d2c9ea32a8f6043a3e06b2a7f8280ef68b /export/bin/src/OpenBSD/g14
> > blob - 1d0cd5fcede71f0495a271a9d06fc9c0ecb16412
> > file + sys/dev/acpi/amdgpio.c
> > --- sys/dev/acpi/amdgpio.c
> > +++ sys/dev/acpi/amdgpio.c
> > @@ -62,13 +62,17 @@ struct amdgpio_softc {
> > struct amdgpio_intrhand *sc_pin_ih;
> >
> > struct acpi_gpio sc_gpio;
> > +
> > + uint32_t *sc_pincfg;
> > };
> >
> > int amdgpio_match(struct device *, void *, void *);
> > void amdgpio_attach(struct device *, struct device *, void *);
> > +int amdgpio_activate(struct device *, int);
> >
> > const struct cfattach amdgpio_ca = {
> > - sizeof(struct amdgpio_softc), amdgpio_match, amdgpio_attach
> > + sizeof(struct amdgpio_softc), amdgpio_match, amdgpio_attach, NULL,
> > + amdgpio_activate
> > };
> >
> > struct cfdriver amdgpio_cd = {
> > @@ -98,6 +102,24 @@ amdgpio_match(struct device *parent, void *match, void
> > return acpi_matchhids(aaa, amdgpio_hids, cf->cf_driver->cd_name);
> > }
> >
> > +int
> > +amdgpio_activate(struct device *self, int act)
> > +{
> > + struct amdgpio_softc *sc = (struct amdgpio_softc *)self;
> > + int rv = 0, i;
> > +
> > + switch (act) {
> > + case DVACT_WAKEUP:
> > + for (i = 0; i < sc->sc_npins; i++) {
> > + if (sc->sc_pincfg[i])
> > + bus_space_write_4(sc->sc_memt, sc->sc_memh,
> > + i * 4, sc->sc_pincfg[i]);
> > + }
> > + }
> > +
> > + return (rv);
> > +}
> > +
> > void
> > amdgpio_attach(struct device *parent, struct device *self, void *aux)
> > {
> > @@ -152,6 +174,8 @@ amdgpio_attach(struct device *parent, struct device *s
> > sc->sc_node->gpio = &sc->sc_gpio;
> >
> > printf(", %d pins\n", sc->sc_npins);
> > + sc->sc_pincfg = malloc(sc->sc_npins * sizeof(uint32_t), M_DEVBUF,
> > + M_WAITOK | M_ZERO);
> >
> > acpi_register_gpio(sc->sc_acpi, sc->sc_node);
> > return;
> > @@ -210,6 +234,8 @@ amdgpio_intr_establish(void *cookie, int pin, int flag
> > reg |= AMDGPIO_CONF_ACTBOTH;
> > reg |= (AMDGPIO_CONF_INT_MASK | AMDGPIO_CONF_INT_EN);
> > bus_space_write_4(sc->sc_memt, sc->sc_memh, pin * 4, reg);
> > +
> > + sc->sc_pincfg[pin] = reg;
> > }
> >
> > int
> >
> >
>
