| Message ID | 20180712152506.11883-1-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] gpiolib-acpi: make sure we trigger edge events at least once on boot | expand |
Hi, On 12-07-18 18:23, Andy Shevchenko wrote: > On Thu, 2018-07-12 at 17:25 +0200, Hans de Goede wrote: >> From: Benjamin Tissoires <benjamin.tissoires@redhat.com> >> >> 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? > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Thanks. > One nitpick below, though I'm fine if you ignore it. > >> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> >> [hdegoede: Document BYT USB mux reliance on initial trigger] >> [hdegoede: Run event handler from a late_initcall, rather then >> immediately] >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v2: >> -Use late_initcall_sync to delay running the handler till other >> drivers >> have been probed, rather then using a delayed_workqueue >> --- >> drivers/gpio/gpiolib-acpi.c | 56 >> ++++++++++++++++++++++++++++++++++++- >> 1 file changed, 55 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c >> index e2232cbcec8b..addd9fecc198 100644 >> --- a/drivers/gpio/gpiolib-acpi.c >> +++ b/drivers/gpio/gpiolib-acpi.c >> @@ -25,6 +25,7 @@ >> >> struct acpi_gpio_event { >> struct list_head node; >> + struct list_head initial_sync_list; >> acpi_handle handle; >> unsigned int pin; >> unsigned int irq; >> @@ -50,6 +51,9 @@ struct acpi_gpio_chip { >> struct list_head events; >> }; >> >> +static LIST_HEAD(acpi_gpio_initial_sync_list); >> +static DEFINE_MUTEX(acpi_gpio_initial_sync_list_lock); >> + >> static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) >> { >> if (!gc->parent) >> @@ -85,6 +89,21 @@ static struct gpio_desc *acpi_get_gpiod(char *path, >> int pin) >> return gpiochip_get_desc(chip, pin); >> } >> >> +static void acpi_gpio_add_to_initial_sync_list(struct acpi_gpio_event >> *event) >> +{ >> + mutex_lock(&acpi_gpio_initial_sync_list_lock); >> + list_add(&event->initial_sync_list, >> &acpi_gpio_initial_sync_list); >> + mutex_unlock(&acpi_gpio_initial_sync_list_lock); >> +} >> + >> +static void acpi_gpio_del_from_initial_sync_list(struct >> acpi_gpio_event *event) >> +{ >> + mutex_lock(&acpi_gpio_initial_sync_list_lock); >> + if (!list_empty(&event->initial_sync_list)) >> + list_del_init(&event->initial_sync_list); >> + mutex_unlock(&acpi_gpio_initial_sync_list_lock); >> +} >> + >> static irqreturn_t acpi_gpio_irq_handler(int irq, void *data) >> { >> struct acpi_gpio_event *event = data; >> @@ -136,7 +155,7 @@ static acpi_status >> acpi_gpiochip_request_interrupt(struct acpi_resource *ares, >> irq_handler_t handler = NULL; >> struct gpio_desc *desc; >> unsigned long irqflags; >> - int ret, pin, irq; >> + int ret, pin, irq, value; >> >> if (!acpi_gpio_get_irq_resource(ares, &agpio)) >> return AE_OK; >> @@ -167,6 +186,8 @@ static acpi_status >> acpi_gpiochip_request_interrupt(struct acpi_resource *ares, >> >> gpiod_direction_input(desc); >> >> + value = gpiod_get_value(desc); >> + >> ret = gpiochip_lock_as_irq(chip, pin); >> if (ret) { >> dev_err(chip->parent, "Failed to lock GPIO as >> interrupt\n"); >> @@ -208,6 +229,7 @@ static acpi_status >> acpi_gpiochip_request_interrupt(struct acpi_resource *ares, >> event->irq = irq; >> event->pin = pin; >> event->desc = desc; >> + INIT_LIST_HEAD(&event->initial_sync_list); >> >> ret = request_threaded_irq(event->irq, NULL, handler, >> irqflags, >> "ACPI:Event", event); >> @@ -222,6 +244,18 @@ static acpi_status >> acpi_gpiochip_request_interrupt(struct acpi_resource *ares, >> enable_irq_wake(irq); >> >> list_add_tail(&event->node, &acpi_gpio->events); >> + >> + /* >> + * Make sure we trigger the initial state of the IRQ when >> using RISING >> + * or FALLING. Note we run the handlers on late_init, the >> AML code >> + * may refer to OperationRegions from other (builtin) drivers >> which >> + * may be probed after us. >> + */ >> + if (handler == acpi_gpio_irq_handler && >> + (((irqflags & IRQF_TRIGGER_RISING) && value == 1) || >> + ((irqflags & IRQF_TRIGGER_FALLING) && value == 0))) >> + acpi_gpio_add_to_initial_sync_list(event); >> + >> return AE_OK; >> >> fail_free_event: >> @@ -294,6 +328,8 @@ void acpi_gpiochip_free_interrupts(struct >> gpio_chip *chip) >> list_for_each_entry_safe_reverse(event, ep, &acpi_gpio- >>> events, node) { >> struct gpio_desc *desc; >> >> + acpi_gpio_del_from_initial_sync_list(event); >> + >> if (irqd_is_wakeup_set(irq_get_irq_data(event->irq))) >> disable_irq_wake(event->irq); >> >> @@ -1158,3 +1194,21 @@ bool acpi_can_fallback_to_crs(struct >> acpi_device *adev, const char *con_id) >> >> return con_id == NULL; >> } >> + >> +/* Sync the initial state of handlers after all builtin drivers have >> probed */ >> +static int acpi_gpio_initial_sync(void) >> +{ >> + struct acpi_gpio_event *event, *ep; >> + >> + mutex_lock(&acpi_gpio_initial_sync_list_lock); >> + list_for_each_entry_safe(event, ep, >> &acpi_gpio_initial_sync_list, >> + initial_sync_list) { >> + acpi_evaluate_object(event->handle, NULL, NULL, >> NULL); >> + list_del_init(&event->initial_sync_list); >> + } >> + mutex_unlock(&acpi_gpio_initial_sync_list_lock); >> + >> + return 0; >> +} > >> +/* We must use _sync so that this runs after the first deferred_probe >> run */ > > I think you may remove _ from above comment. I actually like it with _ better, as that makes clear it is about the _sync postfix to late_initcall where as just sync makes that less clear IMHO. >> +late_initcall_sync(acpi_gpio_initial_sync); Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 12, 2018 at 6:23 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, 2018-07-12 at 17:25 +0200, Hans de Goede wrote: > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > 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. Cheers, Benjamin > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > One nitpick below, though I'm fine if you ignore it. > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > [hdegoede: Document BYT USB mux reliance on initial trigger] > > [hdegoede: Run event handler from a late_initcall, rather then > > immediately] > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > Changes in v2: > > -Use late_initcall_sync to delay running the handler till other > > drivers > > have been probed, rather then using a delayed_workqueue > > --- > > drivers/gpio/gpiolib-acpi.c | 56 > > ++++++++++++++++++++++++++++++++++++- > > 1 file changed, 55 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > > index e2232cbcec8b..addd9fecc198 100644 > > --- a/drivers/gpio/gpiolib-acpi.c > > +++ b/drivers/gpio/gpiolib-acpi.c > > @@ -25,6 +25,7 @@ > > > > struct acpi_gpio_event { > > struct list_head node; > > + struct list_head initial_sync_list; > > acpi_handle handle; > > unsigned int pin; > > unsigned int irq; > > @@ -50,6 +51,9 @@ struct acpi_gpio_chip { > > struct list_head events; > > }; > > > > +static LIST_HEAD(acpi_gpio_initial_sync_list); > > +static DEFINE_MUTEX(acpi_gpio_initial_sync_list_lock); > > + > > static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) > > { > > if (!gc->parent) > > @@ -85,6 +89,21 @@ static struct gpio_desc *acpi_get_gpiod(char *path, > > int pin) > > return gpiochip_get_desc(chip, pin); > > } > > > > +static void acpi_gpio_add_to_initial_sync_list(struct acpi_gpio_event > > *event) > > +{ > > + mutex_lock(&acpi_gpio_initial_sync_list_lock); > > + list_add(&event->initial_sync_list, > > &acpi_gpio_initial_sync_list); > > + mutex_unlock(&acpi_gpio_initial_sync_list_lock); > > +} > > + > > +static void acpi_gpio_del_from_initial_sync_list(struct > > acpi_gpio_event *event) > > +{ > > + mutex_lock(&acpi_gpio_initial_sync_list_lock); > > + if (!list_empty(&event->initial_sync_list)) > > + list_del_init(&event->initial_sync_list); > > + mutex_unlock(&acpi_gpio_initial_sync_list_lock); > > +} > > + > > static irqreturn_t acpi_gpio_irq_handler(int irq, void *data) > > { > > struct acpi_gpio_event *event = data; > > @@ -136,7 +155,7 @@ static acpi_status > > acpi_gpiochip_request_interrupt(struct acpi_resource *ares, > > irq_handler_t handler = NULL; > > struct gpio_desc *desc; > > unsigned long irqflags; > > - int ret, pin, irq; > > + int ret, pin, irq, value; > > > > if (!acpi_gpio_get_irq_resource(ares, &agpio)) > > return AE_OK; > > @@ -167,6 +186,8 @@ static acpi_status > > acpi_gpiochip_request_interrupt(struct acpi_resource *ares, > > > > gpiod_direction_input(desc); > > > > + value = gpiod_get_value(desc); > > + > > ret = gpiochip_lock_as_irq(chip, pin); > > if (ret) { > > dev_err(chip->parent, "Failed to lock GPIO as > > interrupt\n"); > > @@ -208,6 +229,7 @@ static acpi_status > > acpi_gpiochip_request_interrupt(struct acpi_resource *ares, > > event->irq = irq; > > event->pin = pin; > > event->desc = desc; > > + INIT_LIST_HEAD(&event->initial_sync_list); > > > > ret = request_threaded_irq(event->irq, NULL, handler, > > irqflags, > > "ACPI:Event", event); > > @@ -222,6 +244,18 @@ static acpi_status > > acpi_gpiochip_request_interrupt(struct acpi_resource *ares, > > enable_irq_wake(irq); > > > > list_add_tail(&event->node, &acpi_gpio->events); > > + > > + /* > > + * Make sure we trigger the initial state of the IRQ when > > using RISING > > + * or FALLING. Note we run the handlers on late_init, the > > AML code > > + * may refer to OperationRegions from other (builtin) drivers > > which > > + * may be probed after us. > > + */ > > + if (handler == acpi_gpio_irq_handler && > > + (((irqflags & IRQF_TRIGGER_RISING) && value == 1) || > > + ((irqflags & IRQF_TRIGGER_FALLING) && value == 0))) > > + acpi_gpio_add_to_initial_sync_list(event); > > + > > return AE_OK; > > > > fail_free_event: > > @@ -294,6 +328,8 @@ void acpi_gpiochip_free_interrupts(struct > > gpio_chip *chip) > > list_for_each_entry_safe_reverse(event, ep, &acpi_gpio- > > >events, node) { > > struct gpio_desc *desc; > > > > + acpi_gpio_del_from_initial_sync_list(event); > > + > > if (irqd_is_wakeup_set(irq_get_irq_data(event->irq))) > > disable_irq_wake(event->irq); > > > > @@ -1158,3 +1194,21 @@ bool acpi_can_fallback_to_crs(struct > > acpi_device *adev, const char *con_id) > > > > return con_id == NULL; > > } > > + > > +/* Sync the initial state of handlers after all builtin drivers have > > probed */ > > +static int acpi_gpio_initial_sync(void) > > +{ > > + struct acpi_gpio_event *event, *ep; > > + > > + mutex_lock(&acpi_gpio_initial_sync_list_lock); > > + list_for_each_entry_safe(event, ep, > > &acpi_gpio_initial_sync_list, > > + initial_sync_list) { > > + acpi_evaluate_object(event->handle, NULL, NULL, > > NULL); > > + list_del_init(&event->initial_sync_list); > > + } > > + mutex_unlock(&acpi_gpio_initial_sync_list_lock); > > + > > + return 0; > > +} > > > +/* We must use _sync so that this runs after the first deferred_probe > > run */ > > I think you may remove _ from above comment. > > > +late_initcall_sync(acpi_gpio_initial_sync); > > -- > Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 13, 2018 at 10:13 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Thu, Jul 12, 2018 at 6:23 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Thu, 2018-07-12 at 17:25 +0200, Hans de Goede wrote: > > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > > > 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). I just checked, and surface3_wmi has another capability: it helps setting up the touchscreen/stylus when the tablet is booted with the cover closed. So maybe we should be able to strip out the LID part of surface3_wmi, but we should keep the rest anyway. One last thing, this patch also enable the charging capability of the tablet. With a kernel that doesn't have it, but which has the MSHW0011 driver I reversed engineer, the ADP will be marked as plugged in, but the tablet won't charge (and the battery will not be marked as charging). With it, everything goes back to normal :) Cheers, Benjamin > > > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > One nitpick below, though I'm fine if you ignore it. > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > [hdegoede: Document BYT USB mux reliance on initial trigger] > > > [hdegoede: Run event handler from a late_initcall, rather then > > > immediately] > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > --- > > > Changes in v2: > > > -Use late_initcall_sync to delay running the handler till other > > > drivers > > > have been probed, rather then using a delayed_workqueue > > > --- > > > drivers/gpio/gpiolib-acpi.c | 56 > > > ++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 55 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > > > index e2232cbcec8b..addd9fecc198 100644 > > > --- a/drivers/gpio/gpiolib-acpi.c > > > +++ b/drivers/gpio/gpiolib-acpi.c > > > @@ -25,6 +25,7 @@ > > > > > > struct acpi_gpio_event { > > > struct list_head node; > > > + struct list_head initial_sync_list; > > > acpi_handle handle; > > > unsigned int pin; > > > unsigned int irq; > > > @@ -50,6 +51,9 @@ struct acpi_gpio_chip { > > > struct list_head events; > > > }; > > > > > > +static LIST_HEAD(acpi_gpio_initial_sync_list); > > > +static DEFINE_MUTEX(acpi_gpio_initial_sync_list_lock); > > > + > > > static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) > > > { > > > if (!gc->parent) > > > @@ -85,6 +89,21 @@ static struct gpio_desc *acpi_get_gpiod(char *path, > > > int pin) > > > return gpiochip_get_desc(chip, pin); > > > } > > > > > > +static void acpi_gpio_add_to_initial_sync_list(struct acpi_gpio_event > > > *event) > > > +{ > > > + mutex_lock(&acpi_gpio_initial_sync_list_lock); > > > + list_add(&event->initial_sync_list, > > > &acpi_gpio_initial_sync_list); > > > + mutex_unlock(&acpi_gpio_initial_sync_list_lock); > > > +} > > > + > > > +static void acpi_gpio_del_from_initial_sync_list(struct > > > acpi_gpio_event *event) > > > +{ > > > + mutex_lock(&acpi_gpio_initial_sync_list_lock); > > > + if (!list_empty(&event->initial_sync_list)) > > > + list_del_init(&event->initial_sync_list); > > > + mutex_unlock(&acpi_gpio_initial_sync_list_lock); > > > +} > > > + > > > static irqreturn_t acpi_gpio_irq_handler(int irq, void *data) > > > { > > > struct acpi_gpio_event *event = data; > > > @@ -136,7 +155,7 @@ static acpi_status > > > acpi_gpiochip_request_interrupt(struct acpi_resource *ares, > > > irq_handler_t handler = NULL; > > > struct gpio_desc *desc; > > > unsigned long irqflags; > > > - int ret, pin, irq; > > > + int ret, pin, irq, value; > > > > > > if (!acpi_gpio_get_irq_resource(ares, &agpio)) > > > return AE_OK; > > > @@ -167,6 +186,8 @@ static acpi_status > > > acpi_gpiochip_request_interrupt(struct acpi_resource *ares, > > > > > > gpiod_direction_input(desc); > > > > > > + value = gpiod_get_value(desc); > > > + > > > ret = gpiochip_lock_as_irq(chip, pin); > > > if (ret) { > > > dev_err(chip->parent, "Failed to lock GPIO as > > > interrupt\n"); > > > @@ -208,6 +229,7 @@ static acpi_status > > > acpi_gpiochip_request_interrupt(struct acpi_resource *ares, > > > event->irq = irq; > > > event->pin = pin; > > > event->desc = desc; > > > + INIT_LIST_HEAD(&event->initial_sync_list); > > > > > > ret = request_threaded_irq(event->irq, NULL, handler, > > > irqflags, > > > "ACPI:Event", event); > > > @@ -222,6 +244,18 @@ static acpi_status > > > acpi_gpiochip_request_interrupt(struct acpi_resource *ares, > > > enable_irq_wake(irq); > > > > > > list_add_tail(&event->node, &acpi_gpio->events); > > > + > > > + /* > > > + * Make sure we trigger the initial state of the IRQ when > > > using RISING > > > + * or FALLING. Note we run the handlers on late_init, the > > > AML code > > > + * may refer to OperationRegions from other (builtin) drivers > > > which > > > + * may be probed after us. > > > + */ > > > + if (handler == acpi_gpio_irq_handler && > > > + (((irqflags & IRQF_TRIGGER_RISING) && value == 1) || > > > + ((irqflags & IRQF_TRIGGER_FALLING) && value == 0))) > > > + acpi_gpio_add_to_initial_sync_list(event); > > > + > > > return AE_OK; > > > > > > fail_free_event: > > > @@ -294,6 +328,8 @@ void acpi_gpiochip_free_interrupts(struct > > > gpio_chip *chip) > > > list_for_each_entry_safe_reverse(event, ep, &acpi_gpio- > > > >events, node) { > > > struct gpio_desc *desc; > > > > > > + acpi_gpio_del_from_initial_sync_list(event); > > > + > > > if (irqd_is_wakeup_set(irq_get_irq_data(event->irq))) > > > disable_irq_wake(event->irq); > > > > > > @@ -1158,3 +1194,21 @@ bool acpi_can_fallback_to_crs(struct > > > acpi_device *adev, const char *con_id) > > > > > > return con_id == NULL; > > > } > > > + > > > +/* Sync the initial state of handlers after all builtin drivers have > > > probed */ > > > +static int acpi_gpio_initial_sync(void) > > > +{ > > > + struct acpi_gpio_event *event, *ep; > > > + > > > + mutex_lock(&acpi_gpio_initial_sync_list_lock); > > > + list_for_each_entry_safe(event, ep, > > > &acpi_gpio_initial_sync_list, > > > + initial_sync_list) { > > > + acpi_evaluate_object(event->handle, NULL, NULL, > > > NULL); > > > + list_del_init(&event->initial_sync_list); > > > + } > > > + mutex_unlock(&acpi_gpio_initial_sync_list_lock); > > > + > > > + return 0; > > > +} > > > > > +/* We must use _sync so that this runs after the first deferred_probe > > > run */ > > > > I think you may remove _ from above comment. > > > > > +late_initcall_sync(acpi_gpio_initial_sync); > > > > -- > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 12, 2018 at 05:25:06PM +0200, Hans de Goede wrote: > From: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > 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) > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > [hdegoede: Document BYT USB mux reliance on initial trigger] > [hdegoede: Run event handler from a late_initcall, rather then immediately] > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 13, 2018 at 5:52 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Fri, Jul 13, 2018 at 10:13 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > On Thu, Jul 12, 2018 at 6:23 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Thu, 2018-07-12 at 17:25 +0200, Hans de Goede wrote: > > > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > > > > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2018-07-13 at 17:52 +0200, Benjamin Tissoires wrote: > On Fri, Jul 13, 2018 at 10:13 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > On Thu, Jul 12, 2018 at 6:23 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Thu, 2018-07-12 at 17:25 +0200, Hans de Goede wrote: > > > 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). > > I just checked, and surface3_wmi has another capability: it helps > setting up the touchscreen/stylus when the tablet is booted with the > cover closed. So maybe we should be able to strip out the LID part of > surface3_wmi, but we should keep the rest anyway. > > One last thing, this patch also enable the charging capability of the > tablet. With a kernel that doesn't have it, but which has the MSHW0011 > driver I reversed engineer, the ADP will be marked as plugged in, but > the tablet won't charge (and the battery will not be marked as > charging). With it, everything goes back to normal :) Very nice! Thanks a lot for confirming!
On Thu, Jul 12, 2018 at 5:25 PM Hans de Goede <hdegoede@redhat.com> wrote: > From: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > 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) > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > [hdegoede: Document BYT USB mux reliance on initial trigger] > [hdegoede: Run event handler from a late_initcall, rather then immediately] > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > -Use late_initcall_sync to delay running the handler till other drivers > have been probed, rather then using a delayed_workqueue I've applied this for fixes. Sorry for the delay. I'm technically on vacation. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index e2232cbcec8b..addd9fecc198 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -25,6 +25,7 @@ struct acpi_gpio_event { struct list_head node; + struct list_head initial_sync_list; acpi_handle handle; unsigned int pin; unsigned int irq; @@ -50,6 +51,9 @@ struct acpi_gpio_chip { struct list_head events; }; +static LIST_HEAD(acpi_gpio_initial_sync_list); +static DEFINE_MUTEX(acpi_gpio_initial_sync_list_lock); + static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) { if (!gc->parent) @@ -85,6 +89,21 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin) return gpiochip_get_desc(chip, pin); } +static void acpi_gpio_add_to_initial_sync_list(struct acpi_gpio_event *event) +{ + mutex_lock(&acpi_gpio_initial_sync_list_lock); + list_add(&event->initial_sync_list, &acpi_gpio_initial_sync_list); + mutex_unlock(&acpi_gpio_initial_sync_list_lock); +} + +static void acpi_gpio_del_from_initial_sync_list(struct acpi_gpio_event *event) +{ + mutex_lock(&acpi_gpio_initial_sync_list_lock); + if (!list_empty(&event->initial_sync_list)) + list_del_init(&event->initial_sync_list); + mutex_unlock(&acpi_gpio_initial_sync_list_lock); +} + static irqreturn_t acpi_gpio_irq_handler(int irq, void *data) { struct acpi_gpio_event *event = data; @@ -136,7 +155,7 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares, irq_handler_t handler = NULL; struct gpio_desc *desc; unsigned long irqflags; - int ret, pin, irq; + int ret, pin, irq, value; if (!acpi_gpio_get_irq_resource(ares, &agpio)) return AE_OK; @@ -167,6 +186,8 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares, gpiod_direction_input(desc); + value = gpiod_get_value(desc); + ret = gpiochip_lock_as_irq(chip, pin); if (ret) { dev_err(chip->parent, "Failed to lock GPIO as interrupt\n"); @@ -208,6 +229,7 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares, event->irq = irq; event->pin = pin; event->desc = desc; + INIT_LIST_HEAD(&event->initial_sync_list); ret = request_threaded_irq(event->irq, NULL, handler, irqflags, "ACPI:Event", event); @@ -222,6 +244,18 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares, enable_irq_wake(irq); list_add_tail(&event->node, &acpi_gpio->events); + + /* + * Make sure we trigger the initial state of the IRQ when using RISING + * or FALLING. Note we run the handlers on late_init, the AML code + * may refer to OperationRegions from other (builtin) drivers which + * may be probed after us. + */ + if (handler == acpi_gpio_irq_handler && + (((irqflags & IRQF_TRIGGER_RISING) && value == 1) || + ((irqflags & IRQF_TRIGGER_FALLING) && value == 0))) + acpi_gpio_add_to_initial_sync_list(event); + return AE_OK; fail_free_event: @@ -294,6 +328,8 @@ void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) list_for_each_entry_safe_reverse(event, ep, &acpi_gpio->events, node) { struct gpio_desc *desc; + acpi_gpio_del_from_initial_sync_list(event); + if (irqd_is_wakeup_set(irq_get_irq_data(event->irq))) disable_irq_wake(event->irq); @@ -1158,3 +1194,21 @@ bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id) return con_id == NULL; } + +/* Sync the initial state of handlers after all builtin drivers have probed */ +static int acpi_gpio_initial_sync(void) +{ + struct acpi_gpio_event *event, *ep; + + mutex_lock(&acpi_gpio_initial_sync_list_lock); + list_for_each_entry_safe(event, ep, &acpi_gpio_initial_sync_list, + initial_sync_list) { + acpi_evaluate_object(event->handle, NULL, NULL, NULL); + list_del_init(&event->initial_sync_list); + } + mutex_unlock(&acpi_gpio_initial_sync_list_lock); + + return 0; +} +/* We must use _sync so that this runs after the first deferred_probe run */ +late_initcall_sync(acpi_gpio_initial_sync);