On Tue, Nov 19, 2019 at 12:12:35PM +0100, Hans de Goede wrote:
> Hi,
>
> On 19-11-2019 09:26, Mika Westerberg wrote:
> > On Mon, Nov 18, 2019 at 04:35:56PM +0100, Hans de Goede wrote:
> > > The Acer Switch 10 SW5-032 _LID method is quite broken, it looks like this:
> > >
> > > Method (_LID, 0, NotSerialized) // _LID: Lid Status
> > > {
> > > If ((STAS & One))
> > > {
> > > Local0 = One
> > > PBCG |= 0x05000000
> > > HMCG |= 0x05000000
> > > }
> > > Else
> > > {
> > > Local0 = Zero
> > > PBCG &= 0xF0FFFFFF
> > > HMCG &= 0xF0FFFFFF
> > > }
> > >
> > > ^^PCI0.GFX0.CLID = Local0
> > > Return (Local0)
> > > }
> > >
> > > The problem here is the accesses to the PBCG and HMCG, these are the
> > > pinconf0 registers for the power, resp. the home button GPIO,
> > > e.g. PBCG is declared as:
> > >
> > > OperationRegion (PWBT, SystemMemory, 0xFED0E080, 0x10)
> > > Field (PWBT, DWordAcc, NoLock, Preserve)
> > > {
> > > PBCG, 32,
> > > PBV1, 32,
> > > PBSA, 32,
> > > PBV2, 32
> > > }
> > >
> > > Where 0xFED0E000 is the base address of the GPO2 device and 0x80 is
> > > the offset for the pin used for the powerbutton.
> > >
> > > The problem here is this line in _LID:
> > > PBCG |= 0x05000000
> > >
> > > This changes the trigger flags of the GPIO, changing when it generates
> > > interrupts. Note it does not clear the original flags. Linux uses an
> > > edge triggered interrupt on both positive and negative edges. This |=
> > > adds the BYT_TRIG_LVL flag to this, so now it is turned into a level
> > > interrupt which fires both when low and high, iow it simply always
> > > fires leading to an interrupt storm, the tablet immediately waking up
> > > from suspend again, etc.
> >
> > Hmm, does it work in Windows?
>
> I bought this machine 2nd hand and the Windows install is broken
> (the eMMC is dead) so I do not know with 100% certainty.
>
> I guess it does work in Windows, I would assume so at least. I suspect
> that the Windows driver for "PNP0C40" GPIO buttons devices uses level
> interrupts only listening for presses which would match the "5" in the
> mask. Note that that would very much go against the ACPI description,
> which describes the 4 GPIOs for pwrbutton/home/vol+/vol- as follows:
>
> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
> {
> Name (RBUF, ResourceTemplate ()
> {
> GpioInt (Edge, ActiveBoth, ExclusiveAndWake, PullDefault, 0x0000,
> "\\_SB.GPO2", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0010
> }
> GpioInt (Edge, ActiveBoth, ExclusiveAndWake, PullDefault, 0x0000,
> "\\_SB.GPO2", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0015
> }
> GpioInt (Edge, ActiveBoth, ExclusiveAndWake, PullDefault, 0x0000,
> "\\_SB.GPO0", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0001
> }
> GpioInt (Edge, ActiveBoth, ExclusiveAndWake, PullDefault, 0x0000,
> "\\_SB.GPO0", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0000
> }
> })
> Return (RBUF) /* \_SB_.TBAD._CRS.RBUF */
> }
>
> Notice how all GPIOs are specified as GpioInt's which are active on
> both edges and this is what the linux gpio_keys driver uses.
OK thanks for the details.
> Working around this is not impossible, but it will be quite ugly and given
> the age of the machine IMHO not worth it.
I agree.
> I've also found out that I need a DSDT override to be able to control
> the LCD backlight, this is controlled by the 1st PWM controller in the
> SoC LPSS block, which is normally enumerated through ACPI but the
> entire Device (PWM1) {} block is missing from the DSDT :| Adding it
> from similar hardware fixes things and makes the backlight
> controllable. TL;DR: it seems that this is one of the rare cased where
> people who want to run Linux will need to do a manual DSDT override :|
>
> When they do that override they can also fix the _LID method and
> then re-enable LID functionality on the kernel commandline overriding
> this DMI quirk.
>
> I will probably do a blog post on this (some people have asked me
> to do some blogposts about how to analyze DSDT-s, this will be a nice
> example) and add a link to the DSDT override to the blogpost, I believe
> that this is the best we can do for users of this device.
>
> In the meantime this quirk at least avoids the interrupt storm making
> the device mostly usable even without the DSDT override.
Yup, makes sense.
