Re: [PATCH v2] gpiolib-acpi: make sure we trigger edge events at least once on boot

[Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>]

On Fri, Jul 13, 2018 at 5:52 PM Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> wrote:
>
> On Fri, Jul 13, 2018 at 10:13 AM Benjamin Tissoires
> <benjamin.tissoires@xxxxxxxxxx> wrote:
> >
> > On Thu, Jul 12, 2018 at 6:23 PM Andy Shevchenko
> > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > >
> > > On Thu, 2018-07-12 at 17:25 +0200, Hans de Goede wrote:
> > > > From: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> > > >
> > > > On some systems using edge triggered ACPI Event Interrupts, the
> > > > initial
> > > > state at boot is not setup by the firmware, instead relying on the
> > > > edge
> > > > irq event handler running at least once to setup the initial state.
> > > >
> > > > 2 known examples of this are:
> > > >
> > > > 1) The Surface 3 has its _LID state controlled by an ACPI operation
> > > > region
> > > >  triggered by a GPIO event:
> > > >
> > > >  OperationRegion (GPOR, GeneralPurposeIo, Zero, One)
> > > >  Field (GPOR, ByteAcc, NoLock, Preserve)
> > > >  {
> > > >      Connection (
> > > >          GpioIo (Shared, PullNone, 0x0000, 0x0000, IoRestrictionNone,
> > > >              "\\_SB.GPO0", 0x00, ResourceConsumer, ,
> > > >              )
> > > >              {   // Pin list
> > > >                  0x004C
> > > >              }
> > > >      ),
> > > >      HELD,   1
> > > >  }
> > > >
> > > >  Method (_E4C, 0, Serialized)  // _Exx: Edge-Triggered GPE
> > > >  {
> > > >      If ((HELD == One))
> > > >      {
> > > >          ^^LID.LIDB = One
> > > >      }
> > > >      Else
> > > >      {
> > > >          ^^LID.LIDB = Zero
> > > >          Notify (LID, 0x80) // Status Change
> > > >      }
> > > >
> > > >      Notify (^^PCI0.SPI1.NTRG, One) // Device Check
> > > >  }
> > > >
> > > >  Currently, the state of LIDB is wrong until the user actually closes
> > > > or
> > > >  open the cover. We need to trigger the GPIO event once to update the
> > > >  internal ACPI state.
> > > >
> > > >  Coincidentally, this also enables the Surface 2 integrated HID sensor
> > > > hub
> > > >  which also requires an ACPI gpio operation region to start
> > > > initialization.
> > > >
> > > > 2) Various Bay Trail based tablets come with an external USB mux and
> > > >  TI T1210B USB phy to enable USB gadget mode. The mux is controlled by
> > > > a
> > > >  GPIO which is controlled by an edge triggered ACPI Event Interrupt
> > > > which
> > > >  monitors the micro-USB ID pin.
> > > >
> > > >  When the tablet is connected to a PC (or no cable is plugged in), the
> > > > ID
> > > >  pin is high and the tablet should be in gadget mode. But the GPIO
> > > >  controlling the mux is initialized by the firmware so that the USB
> > > > data
> > > >  lines are muxed to the host controller.
> > > >
> > > >  This means that if the user wants to use gadget mode, the user needs
> > > > to
> > > >  first plug in a host-cable to force the ID pin low and then unplug it
> > > >  and connect the tablet to a PC, to get the ACPI event handler to run
> > > > and
> > > >  switch the mux to device mode,
> > > >
> > > > This commit fixes both by running the event-handler once on boot.
> > > >
> > > > Note that the running of the event-handler is done from a
> > > > late_initcall,
> > > > this is done because the handler AML code may rely on OperationRegions
> > > > registered by other builtin drivers. This avoids errors like these:
> > > >
> > > > [    0.133026] ACPI Error: No handler for Region [XSCG]
> > > > ((____ptrval____)) [GenericSerialBus] (20180531/evregion-132)
> > > > [    0.133036] ACPI Error: Region GenericSerialBus (ID=9) has no
> > > > handler (20180531/exfldio-265)
> > > > [    0.133046] ACPI Error: Method parse/execution failed
> > > > \_SB.GPO2._E12, AE_NOT_EXIST (20180531/psparse-516)
> > > >
> > >
> > > Yep, this version I like more.
> > >
> > > Benjamin, for sake of testing is it possible to revert (temporary) the
> > > change you mentioned for _LID and test if this helps as well as stated?
> >
> > I'll try to do it today, but I can't guarantee if it will be done
> > today. IIRC, I have been told that the Surface 3 was having
> > difficulties with recent kernels, so crossing fingers here.
> >
>
> after a lot of efforts, I confirm that the latest version from Hans
> fixes the suspend loop issue without surface3_wmi.
> I had to test the patch on a v4.16 kernel as the v4.18 doesn't boot
> (the i915 complains about not being able to drive the DSI, the
> backlight complains in the same way, and lots of new ACPI errors are
> appearing related to the GPU nodes).

On this note, Bastien pointed me at
https://bugzilla.kernel.org/show_bug.cgi?id=200363 which does indeed
fix the boot on the 4.17+ kernels.

Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html