| Message ID | 20220830231541.1135813-1-rrangel@chromium.org |
|---|---|
| Headers | show |
| Series | acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq | expand |
Interesting... The patch series is here: https://patchwork.kernel.org/project/linux-input/cover/20220830231541.1135813-1-rrangel@chromium.org/ I'll look into why you only got added to 2 of the emails. On Wed, Aug 31, 2022 at 5:52 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Aug 30, 2022 at 05:15:33PM -0600, Raul E Rangel wrote: > > Today, i2c drivers are making the assumption that their IRQs can also > > be used as wake IRQs. This isn't always the case and it can lead to > > spurious wakes. This has recently started to affect AMD Chromebooks. > > With the introduction of > > d62bd5ce12d7 ("pinctrl: amd: Implement irq_set_wake"), the AMD GPIO > > controller gained the capability to set the wake bit on each GPIO. The > > ACPI specification defines two ways to inform the system if a device is > > wake capable: > > 1) The _PRW object defines the GPE that can be used to wake the system. > > 2) Setting ExclusiveAndWake or SharedAndWake in the _CRS GpioInt. > > > > Currently only the first method is supported. The i2c drivers don't have > > any indication that the IRQ is wake capable, so they guess. This causes > > spurious interrupts, for example: > > * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have > > `_PRW` or `ExclusiveAndWake` so that means the device can't wake the > > system. > > * The IRQ line is active level low for this device and is pulled up by > > the power resource defined in `_PR0`/`_PR3`. > > * The i2c driver will (incorrectly) arm the GPIO for wake by calling > > `enable_irq_wake` as part of its suspend hook. > > * ACPI will power down the device since it doesn't have a wake GPE > > associated with it. > > * When the device is powered down, the IRQ line will drop, and it will > > trigger a wake event. > > > > See the following debug log: > > [ 42.335804] PM: Suspending system (s2idle) > > [ 42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable > > [ 42.467736] power-0416 __acpi_power_off : Power resource [PR00] turned off > > [ 42.467739] device_pm-0280 device_set_power : Device [H05D] transitioned to D3cold > > [ 42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd > > [ 42.535293] PM: Wakeup unrelated to ACPI SCI > > [ 42.535294] PM: resume from suspend-to-idle > > > > In order to fix this, we need to take into account the wake capable bit > > defined on the GpioInt. This is accomplished by: > > * Migrating some of the i2c drivers over to using the PM subsystem to > > manage the wake IRQ. max8925-i2c, elants_i2c, and raydium_i2c_ts still > > need to be migrated, I can do that depending on the feedback to this > > patch series. > > * Expose the wake_capable bit from the ACPI GpioInt resource to the > > i2c core. > > * Use the wake_capable bit in the i2c core to call > > `dev_pm_set_wake_irq`. This reuses the existing device tree flow. > > * Make the i2c drivers stop calling `dev_pm_set_wake_irq` since it's now > > handled by the i2c core. > > * Make the ACPI device PM system aware of the wake_irq. This is > > necessary so the device doesn't incorrectly get powered down when a > > wake_irq is enabled. > > > > I've tested this code with various combinations of having _PRW, > > ExclusiveAndWake and power resources all defined or not defined, but it > > would be great if others could test this out on their hardware. > > I have got only cover letter and a single patch (#3). What's going on? > > Note: I'm also reviewer of I涎 DesignWare driver, you really have to > fix your tools / submission process and try again. No review for this > series. > > -- > With Best Regards, > Andy Shevchenko > >
Hi, On 8/31/22 16:37, Raul Rangel wrote: > Interesting... The patch series is here: > https://patchwork.kernel.org/project/linux-input/cover/20220830231541.1135813-1-rrangel@chromium.org/ > > I'll look into why you only got added to 2 of the emails. FWIW I also received the full series without problems. I'll try to reply to this soon-ish, but I have a bit of a patch backlog to process and I'm trying to process the backlog in FIFO order and this is one of the last series in the backlog ... Regards, Hans > > On Wed, Aug 31, 2022 at 5:52 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: >> >> On Tue, Aug 30, 2022 at 05:15:33PM -0600, Raul E Rangel wrote: >>> Today, i2c drivers are making the assumption that their IRQs can also >>> be used as wake IRQs. This isn't always the case and it can lead to >>> spurious wakes. This has recently started to affect AMD Chromebooks. >>> With the introduction of >>> d62bd5ce12d7 ("pinctrl: amd: Implement irq_set_wake"), the AMD GPIO >>> controller gained the capability to set the wake bit on each GPIO. The >>> ACPI specification defines two ways to inform the system if a device is >>> wake capable: >>> 1) The _PRW object defines the GPE that can be used to wake the system. >>> 2) Setting ExclusiveAndWake or SharedAndWake in the _CRS GpioInt. >>> >>> Currently only the first method is supported. The i2c drivers don't have >>> any indication that the IRQ is wake capable, so they guess. This causes >>> spurious interrupts, for example: >>> * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have >>> `_PRW` or `ExclusiveAndWake` so that means the device can't wake the >>> system. >>> * The IRQ line is active level low for this device and is pulled up by >>> the power resource defined in `_PR0`/`_PR3`. >>> * The i2c driver will (incorrectly) arm the GPIO for wake by calling >>> `enable_irq_wake` as part of its suspend hook. >>> * ACPI will power down the device since it doesn't have a wake GPE >>> associated with it. >>> * When the device is powered down, the IRQ line will drop, and it will >>> trigger a wake event. >>> >>> See the following debug log: >>> [ 42.335804] PM: Suspending system (s2idle) >>> [ 42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable >>> [ 42.467736] power-0416 __acpi_power_off : Power resource [PR00] turned off >>> [ 42.467739] device_pm-0280 device_set_power : Device [H05D] transitioned to D3cold >>> [ 42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd >>> [ 42.535293] PM: Wakeup unrelated to ACPI SCI >>> [ 42.535294] PM: resume from suspend-to-idle >>> >>> In order to fix this, we need to take into account the wake capable bit >>> defined on the GpioInt. This is accomplished by: >>> * Migrating some of the i2c drivers over to using the PM subsystem to >>> manage the wake IRQ. max8925-i2c, elants_i2c, and raydium_i2c_ts still >>> need to be migrated, I can do that depending on the feedback to this >>> patch series. >>> * Expose the wake_capable bit from the ACPI GpioInt resource to the >>> i2c core. >>> * Use the wake_capable bit in the i2c core to call >>> `dev_pm_set_wake_irq`. This reuses the existing device tree flow. >>> * Make the i2c drivers stop calling `dev_pm_set_wake_irq` since it's now >>> handled by the i2c core. >>> * Make the ACPI device PM system aware of the wake_irq. This is >>> necessary so the device doesn't incorrectly get powered down when a >>> wake_irq is enabled. >>> >>> I've tested this code with various combinations of having _PRW, >>> ExclusiveAndWake and power resources all defined or not defined, but it >>> would be great if others could test this out on their hardware. >> >> I have got only cover letter and a single patch (#3). What's going on? >> >> Note: I'm also reviewer of I涎 DesignWare driver, you really have to >> fix your tools / submission process and try again. No review for this >> series. >> >> -- >> With Best Regards, >> Andy Shevchenko >> >> >
On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote: > > The Elan I2C touchpad driver is currently manually managing the wake > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake > and instead relies on the PM subsystem. This is done by calling > dev_pm_set_wake_irq. > > i2c_device_probe already calls dev_pm_set_wake_irq when using device > tree, so it's only required when using ACPI. The net result is that this > change should be a no-op. i2c_device_remove also already calls > dev_pm_clear_wake_irq, so we don't need to do that in this driver. > > I tested this on an ACPI system where the touchpad doesn't have _PRW > defined. I verified I can still wake the system and that the wake source > was the touchpad IRQ GPIO. > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> I like this a lot, but the assumption in the wakeirq code is that the IRQ in question will be dedicated for signaling wakeup. Does it hold here? > --- > > drivers/input/mouse/elan_i2c_core.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c > index e1758d5ffe4218..7d997d2b56436b 100644 > --- a/drivers/input/mouse/elan_i2c_core.c > +++ b/drivers/input/mouse/elan_i2c_core.c > @@ -33,6 +33,7 @@ > #include <linux/jiffies.h> > #include <linux/completion.h> > #include <linux/of.h> > +#include <linux/pm_wakeirq.h> > #include <linux/property.h> > #include <linux/regulator/consumer.h> > #include <asm/unaligned.h> > @@ -86,8 +87,6 @@ struct elan_tp_data { > u16 fw_page_size; > u32 fw_signature_address; > > - bool irq_wake; > - > u8 min_baseline; > u8 max_baseline; > bool baseline_ready; > @@ -1337,8 +1336,10 @@ static int elan_probe(struct i2c_client *client, > * Systems using device tree should set up wakeup via DTS, > * the rest will configure device as wakeup source by default. > */ > - if (!dev->of_node) > + if (!dev->of_node) { > device_init_wakeup(dev, true); > + dev_pm_set_wake_irq(dev, client->irq); > + } > > return 0; > } > @@ -1362,8 +1363,6 @@ static int __maybe_unused elan_suspend(struct device *dev) > > if (device_may_wakeup(dev)) { > ret = elan_sleep(data); > - /* Enable wake from IRQ */ > - data->irq_wake = (enable_irq_wake(client->irq) == 0); > } else { > ret = elan_set_power(data, false); > if (ret) > @@ -1394,9 +1393,6 @@ static int __maybe_unused elan_resume(struct device *dev) > dev_err(dev, "error %d enabling regulator\n", error); > goto err; > } > - } else if (data->irq_wake) { > - disable_irq_wake(client->irq); > - data->irq_wake = false; > } > > error = elan_set_power(data, true); > -- > 2.37.2.672.g94769d06f0-goog >
On Wed, Aug 31, 2022 at 12:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote: > > > > The Elan I2C touchpad driver is currently manually managing the wake > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake > > and instead relies on the PM subsystem. This is done by calling > > dev_pm_set_wake_irq. > > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device > > tree, so it's only required when using ACPI. The net result is that this > > change should be a no-op. i2c_device_remove also already calls > > dev_pm_clear_wake_irq, so we don't need to do that in this driver. > > > > I tested this on an ACPI system where the touchpad doesn't have _PRW > > defined. I verified I can still wake the system and that the wake source > > was the touchpad IRQ GPIO. > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> > > I like this a lot, but the assumption in the wakeirq code is that the > IRQ in question will be dedicated for signaling wakeup. Does it hold > here? The wakeirq code defines two methods: `dev_pm_set_wake_irq` and `dev_pm_set_dedicated_wake_irq`. The latter is used when you have a dedicated wakeup signal. In this driver it's currently assumed that the IRQ and the wake IRQ are the same, so I used `dev_pm_set_wake_irq`. This change in theory also fixes a bug where you define a dedicated wake irq in DT, but then the driver enables the `client->irq` as a wake source. In practice this doesn't happen since the elan touchpads only have a single IRQ line. > > > --- > > > > drivers/input/mouse/elan_i2c_core.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c > > index e1758d5ffe4218..7d997d2b56436b 100644 > > --- a/drivers/input/mouse/elan_i2c_core.c > > +++ b/drivers/input/mouse/elan_i2c_core.c > > @@ -33,6 +33,7 @@ > > #include <linux/jiffies.h> > > #include <linux/completion.h> > > #include <linux/of.h> > > +#include <linux/pm_wakeirq.h> > > #include <linux/property.h> > > #include <linux/regulator/consumer.h> > > #include <asm/unaligned.h> > > @@ -86,8 +87,6 @@ struct elan_tp_data { > > u16 fw_page_size; > > u32 fw_signature_address; > > > > - bool irq_wake; > > - > > u8 min_baseline; > > u8 max_baseline; > > bool baseline_ready; > > @@ -1337,8 +1336,10 @@ static int elan_probe(struct i2c_client *client, > > * Systems using device tree should set up wakeup via DTS, > > * the rest will configure device as wakeup source by default. > > */ > > - if (!dev->of_node) > > + if (!dev->of_node) { > > device_init_wakeup(dev, true); > > + dev_pm_set_wake_irq(dev, client->irq); > > + } > > > > return 0; > > } > > @@ -1362,8 +1363,6 @@ static int __maybe_unused elan_suspend(struct device *dev) > > > > if (device_may_wakeup(dev)) { > > ret = elan_sleep(data); > > - /* Enable wake from IRQ */ > > - data->irq_wake = (enable_irq_wake(client->irq) == 0); > > } else { > > ret = elan_set_power(data, false); > > if (ret) > > @@ -1394,9 +1393,6 @@ static int __maybe_unused elan_resume(struct device *dev) > > dev_err(dev, "error %d enabling regulator\n", error); > > goto err; > > } > > - } else if (data->irq_wake) { > > - disable_irq_wake(client->irq); > > - data->irq_wake = false; > > } > > > > error = elan_set_power(data, true); > > -- > > 2.37.2.672.g94769d06f0-goog > >
On Wed, Aug 31, 2022 at 8:14 PM Raul Rangel <rrangel@chromium.org> wrote: > > On Wed, Aug 31, 2022 at 12:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote: > > > > > > The Elan I2C touchpad driver is currently manually managing the wake > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake > > > and instead relies on the PM subsystem. This is done by calling > > > dev_pm_set_wake_irq. > > > > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device > > > tree, so it's only required when using ACPI. The net result is that this > > > change should be a no-op. i2c_device_remove also already calls > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver. > > > > > > I tested this on an ACPI system where the touchpad doesn't have _PRW > > > defined. I verified I can still wake the system and that the wake source > > > was the touchpad IRQ GPIO. > > > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> > > > > > > I like this a lot, but the assumption in the wakeirq code is that the > > IRQ in question will be dedicated for signaling wakeup. Does it hold > > here? > > The wakeirq code defines two methods: `dev_pm_set_wake_irq` and > `dev_pm_set_dedicated_wake_irq`. > The latter is used when you have a dedicated wakeup signal. In this > driver it's currently assumed > that the IRQ and the wake IRQ are the same, so I used `dev_pm_set_wake_irq`. > > This change in theory also fixes a bug where you define a dedicated > wake irq in DT, but > then the driver enables the `client->irq` as a wake source. In > practice this doesn't happen > since the elan touchpads only have a single IRQ line. OK, thanks! Please feel free to add Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> to the patch. > > > > > --- > > > > > > drivers/input/mouse/elan_i2c_core.c | 12 ++++-------- > > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c > > > index e1758d5ffe4218..7d997d2b56436b 100644 > > > --- a/drivers/input/mouse/elan_i2c_core.c > > > +++ b/drivers/input/mouse/elan_i2c_core.c > > > @@ -33,6 +33,7 @@ > > > #include <linux/jiffies.h> > > > #include <linux/completion.h> > > > #include <linux/of.h> > > > +#include <linux/pm_wakeirq.h> > > > #include <linux/property.h> > > > #include <linux/regulator/consumer.h> > > > #include <asm/unaligned.h> > > > @@ -86,8 +87,6 @@ struct elan_tp_data { > > > u16 fw_page_size; > > > u32 fw_signature_address; > > > > > > - bool irq_wake; > > > - > > > u8 min_baseline; > > > u8 max_baseline; > > > bool baseline_ready; > > > @@ -1337,8 +1336,10 @@ static int elan_probe(struct i2c_client *client, > > > * Systems using device tree should set up wakeup via DTS, > > > * the rest will configure device as wakeup source by default. > > > */ > > > - if (!dev->of_node) > > > + if (!dev->of_node) { > > > device_init_wakeup(dev, true); > > > + dev_pm_set_wake_irq(dev, client->irq); > > > + } > > > > > > return 0; > > > } > > > @@ -1362,8 +1363,6 @@ static int __maybe_unused elan_suspend(struct device *dev) > > > > > > if (device_may_wakeup(dev)) { > > > ret = elan_sleep(data); > > > - /* Enable wake from IRQ */ > > > - data->irq_wake = (enable_irq_wake(client->irq) == 0); > > > } else { > > > ret = elan_set_power(data, false); > > > if (ret) > > > @@ -1394,9 +1393,6 @@ static int __maybe_unused elan_resume(struct device *dev) > > > dev_err(dev, "error %d enabling regulator\n", error); > > > goto err; > > > } > > > - } else if (data->irq_wake) { > > > - disable_irq_wake(client->irq); > > > - data->irq_wake = false; > > > } > > > > > > error = elan_set_power(data, true); > > > -- > > > 2.37.2.672.g94769d06f0-goog > > >
On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote: > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote: > > > > The Elan I2C touchpad driver is currently manually managing the wake > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake > > and instead relies on the PM subsystem. This is done by calling > > dev_pm_set_wake_irq. > > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device > > tree, so it's only required when using ACPI. The net result is that this > > change should be a no-op. i2c_device_remove also already calls > > dev_pm_clear_wake_irq, so we don't need to do that in this driver. > > > > I tested this on an ACPI system where the touchpad doesn't have _PRW > > defined. I verified I can still wake the system and that the wake source > > was the touchpad IRQ GPIO. > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> > > I like this a lot [...] I also like this a lot, but this assumes that firmware has correct settings for the interrupt... Unfortunately it is not always the case and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry, ect) do not mark interrupt as wakeup: src/mainboard/google/glados/variants/chell/overridetree.cb chip drivers/i2c/generic register "hid" = ""ELAN0000"" register "desc" = ""ELAN Touchpad"" register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)" register "wake" = "GPE0_DW0_05" device i2c 15 on end I assume it should have been ACPI_IRQ_WAKE_LEVEL_LOW for the interrupt to be marked as wakeup. (we do correctly mark GPE as wakeup). So we need to do something about older devices....
On Wed, Aug 31, 2022 at 12:12:41PM -0700, Dmitry Torokhov wrote: > On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote: > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote: > > > > > > The Elan I2C touchpad driver is currently manually managing the wake > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake > > > and instead relies on the PM subsystem. This is done by calling > > > dev_pm_set_wake_irq. > > > > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device > > > tree, so it's only required when using ACPI. The net result is that this > > > change should be a no-op. i2c_device_remove also already calls > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver. > > > > > > I tested this on an ACPI system where the touchpad doesn't have _PRW > > > defined. I verified I can still wake the system and that the wake source > > > was the touchpad IRQ GPIO. > > > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> > > > > I like this a lot [...] > > I also like this a lot, but this assumes that firmware has correct > settings for the interrupt... Unfortunately it is not always the case > and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry, > ect) do not mark interrupt as wakeup: > > src/mainboard/google/glados/variants/chell/overridetree.cb > > chip drivers/i2c/generic > register "hid" = ""ELAN0000"" > register "desc" = ""ELAN Touchpad"" > register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)" > register "wake" = "GPE0_DW0_05" > device i2c 15 on end > > I assume it should have been ACPI_IRQ_WAKE_LEVEL_LOW for the interrupt > to be marked as wakeup. > > (we do correctly mark GPE as wakeup). > > So we need to do something about older devices.... After re-reading the patch I believe this comment is more applicable to the followup patch to elan_i2c, not this one, which is fine on its own. Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Thanks.
On Wed, Aug 31, 2022 at 1:16 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On Wed, Aug 31, 2022 at 12:12:41PM -0700, Dmitry Torokhov wrote: > > On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote: > > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote: > > > > > > > > The Elan I2C touchpad driver is currently manually managing the wake > > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake > > > > and instead relies on the PM subsystem. This is done by calling > > > > dev_pm_set_wake_irq. > > > > > > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device > > > > tree, so it's only required when using ACPI. The net result is that this > > > > change should be a no-op. i2c_device_remove also already calls > > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver. > > > > > > > > I tested this on an ACPI system where the touchpad doesn't have _PRW > > > > defined. I verified I can still wake the system and that the wake source > > > > was the touchpad IRQ GPIO. > > > > > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> > > > > > > I like this a lot [...] > > > > I also like this a lot, but this assumes that firmware has correct > > settings for the interrupt... Unfortunately it is not always the case > > and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry, > > ect) do not mark interrupt as wakeup: > > > > src/mainboard/google/glados/variants/chell/overridetree.cb > > > > chip drivers/i2c/generic > > register "hid" = ""ELAN0000"" > > register "desc" = ""ELAN Touchpad"" > > register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)" > > register "wake" = "GPE0_DW0_05" > > device i2c 15 on end > > So the above entry specifies the `wake` register. This generates an ACPI _PRW resource. The patch series will actually fix devices like this. Today without this patch series we get two wake events for a device. The ACPI wake GPE specified by the _PRW resource, and the erroneous GPIO wake event. But you bring up a good point. I wrote a quick and dirty script (https://0paste.com/391849) to parse the coreboot device tree entries. Open source firmware is great isn't it? ;) $ find src/mainboard/google/ -iname '*.cb' | xargs awk -f touch.awk -- src/mainboard/google/eve/devicetree.cb 1 chip drivers/i2c/hid register "generic.hid" = ""ACPI0C50"" register "generic.desc" = ""Touchpad"" register "generic.irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)" register "hid_desc_reg_offset" = "0x1" device i2c 49 on end end src/mainboard/google/eve/devicetree.cb 1 chip drivers/i2c/generic register "hid" = ""GOOG0008"" register "desc" = ""Touchpad EC Interface"" device i2c 1e on end end src/mainboard/google/drallion/variants/drallion/devicetree.cb 1 chip drivers/i2c/generic register "hid" = ""ELAN0000"" register "desc" = ""ELAN Touchpad"" register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)" register "probed" = "1" device i2c 2c on end end src/mainboard/google/drallion/variants/drallion/devicetree.cb 1 chip drivers/i2c/generic register "hid" = ""ELAN0000"" register "desc" = ""ELAN Touchpad"" register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)" register "probed" = "1" device i2c 15 on end end src/mainboard/google/sarien/variants/arcada/devicetree.cb 1 chip drivers/i2c/generic register "hid" = ""ELAN0000"" register "desc" = ""ELAN Touchpad"" register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)" register "probed" = "1" device i2c 2c on end end src/mainboard/google/sarien/variants/arcada/devicetree.cb 1 chip drivers/i2c/hid register "generic.hid" = ""PNP0C50"" register "generic.desc" = ""Cirque Touchpad"" register "generic.irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)" register "generic.probed" = "1" register "hid_desc_reg_offset" = "0x20" device i2c 2a on end end src/mainboard/google/sarien/variants/sarien/devicetree.cb 1 chip drivers/i2c/generic register "hid" = ""ELAN0000"" register "desc" = ""ELAN Touchpad"" register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)" register "probed" = "1" device i2c 2c on end end Total Touchpad: 202 Total Wake: 195 Out of all the touchpads defined on ChromeOS it looks like only 4 devices are missing a wake declaration. I omitted touchpanels because ChromeOS doesn't use those as a wake source. chromeos_laptop.c already defines some devices with i2c board_info and it sets the `I2C_CLIENT_WAKE` flag. I'm not sure if this is actually working as expected. `i2c_device_probe` requires a `wakeup` irq to be present in the device tree if the `I2C_CLIENT_WAKE` flag is set, but I'm assuming the device tree was missing wake attributes. Anyway, patches 6, and 7 are the ones that drop the legacy behavior. I can figure out how to add the above boards to chromeos_laptop.c and get the wake attribute plumbed, or I can add something directly to the elan_i2c_core, etc so others can add overrides for their boards there. I'll also send out CLs to fix the device tree configs (not that we would run a FW qual just for this change). > > I assume it should have been ACPI_IRQ_WAKE_LEVEL_LOW for the interrupt > > to be marked as wakeup. > > > > (we do correctly mark GPE as wakeup). > > > > So we need to do something about older devices.... > > After re-reading the patch I believe this comment is more applicable to > the followup patch to elan_i2c, not this one, which is fine on its own. > > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Thanks. > > -- > Dmitry
* Rafael J. Wysocki <rafael@kernel.org> [220831 18:35]: > On Wed, Aug 31, 2022 at 8:14 PM Raul Rangel <rrangel@chromium.org> wrote: > > > > On Wed, Aug 31, 2022 at 12:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote: > > > > > > > > The Elan I2C touchpad driver is currently manually managing the wake > > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake > > > > and instead relies on the PM subsystem. This is done by calling > > > > dev_pm_set_wake_irq. > > > > > > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device > > > > tree, so it's only required when using ACPI. The net result is that this > > > > change should be a no-op. i2c_device_remove also already calls > > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver. > > > > > > > > I tested this on an ACPI system where the touchpad doesn't have _PRW > > > > defined. I verified I can still wake the system and that the wake source > > > > was the touchpad IRQ GPIO. > > > > > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> > > > > > > > > > > I like this a lot, but the assumption in the wakeirq code is that the > > > IRQ in question will be dedicated for signaling wakeup. Does it hold > > > here? > > > > The wakeirq code defines two methods: `dev_pm_set_wake_irq` and > > `dev_pm_set_dedicated_wake_irq`. > > The latter is used when you have a dedicated wakeup signal. In this > > driver it's currently assumed > > that the IRQ and the wake IRQ are the same, so I used `dev_pm_set_wake_irq`. > > > > This change in theory also fixes a bug where you define a dedicated > > wake irq in DT, but > > then the driver enables the `client->irq` as a wake source. In > > practice this doesn't happen > > since the elan touchpads only have a single IRQ line. > > OK, thanks! > > Please feel free to add > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > to the patch. Looks good to me too: Reviewed-by: Tony Lindgren <tony@atomide.com>
On Wed, Aug 31, 2022 at 08:17:23PM -0600, Raul Rangel wrote: > On Wed, Aug 31, 2022 at 1:16 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > On Wed, Aug 31, 2022 at 12:12:41PM -0700, Dmitry Torokhov wrote: > > > On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote: > > > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote: > > > > > > > > > > The Elan I2C touchpad driver is currently manually managing the wake > > > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake > > > > > and instead relies on the PM subsystem. This is done by calling > > > > > dev_pm_set_wake_irq. > > > > > > > > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device > > > > > tree, so it's only required when using ACPI. The net result is that this > > > > > change should be a no-op. i2c_device_remove also already calls > > > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver. > > > > > > > > > > I tested this on an ACPI system where the touchpad doesn't have _PRW > > > > > defined. I verified I can still wake the system and that the wake source > > > > > was the touchpad IRQ GPIO. > > > > > > > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> > > > > > > > > I like this a lot [...] > > > > > > > I also like this a lot, but this assumes that firmware has correct > > > settings for the interrupt... Unfortunately it is not always the case > > > and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry, > > > ect) do not mark interrupt as wakeup: > > > > > > src/mainboard/google/glados/variants/chell/overridetree.cb > > > > > > chip drivers/i2c/generic > > > register "hid" = ""ELAN0000"" > > > register "desc" = ""ELAN Touchpad"" > > > register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)" > > > register "wake" = "GPE0_DW0_05" > > > device i2c 15 on end > > > > > So the above entry specifies the `wake` register. This generates an > ACPI _PRW resource. The patch series will actually fix devices like > this. Today without this patch series we get two wake events for a > device. The ACPI wake GPE specified by the _PRW resource, and the > erroneous GPIO wake event. But you bring up a good point. Does this mean that the example that we currently have in coreboot documentation (Documentation/acpi/devicetree.md) is not correct: device pci 15.0 on chip drivers/i2c/generic register "hid" = ""ELAN0000"" register "desc" = ""ELAN Touchpad"" register "irq" = "ACPI_IRQ_WAKE_LEVEL_LOW(GPP_A21_IRQ)" register "wake" = "GPE0_DW0_21" device i2c 15 on end end end # I2C #0 Doesn't in say that we have both GpioIrq and GPE wakeup methods defined for the same device? > > I wrote a quick and dirty script (https://0paste.com/391849) to parse > the coreboot device tree entries. Open source firmware is great isn't > it? ;) > > $ find src/mainboard/google/ -iname '*.cb' | xargs awk -f touch.awk -- > src/mainboard/google/eve/devicetree.cb ... > src/mainboard/google/sarien/variants/sarien/devicetree.cb > 1 > chip drivers/i2c/generic > register "hid" = ""ELAN0000"" > register "desc" = ""ELAN Touchpad"" > register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)" > register "probed" = "1" > device i2c 2c on end > end > Total Touchpad: 202 > Total Wake: 195 > > Out of all the touchpads defined on ChromeOS it looks like only 4 > devices are missing a wake declaration. I omitted touchpanels because > ChromeOS doesn't use those as a wake source. chromeos_laptop.c already > defines some devices with i2c board_info and it sets the > `I2C_CLIENT_WAKE` flag. I'm not sure if this is actually working as > expected. `i2c_device_probe` requires a `wakeup` irq to be present in > the device tree if the `I2C_CLIENT_WAKE` flag is set, but I'm assuming No it does not. If there is no wakeup IRQ defined of_irq_get_byname() will return an error and we'll take the "else if (client->irq > 0)" branch and will set up client->irq as the wakeup irq. > the device tree was missing wake attributes. > > Anyway, patches 6, and 7 are the ones that drop the legacy behavior. I > can figure out how to add the above boards to chromeos_laptop.c and > get the wake attribute plumbed, or I can add something directly to the > elan_i2c_core, etc so others can add overrides for their boards there. > I'll also send out CLs to fix the device tree configs (not that we > would run a FW qual just for this change). My preference is to limit board-specific hacks in drivers if we can, so adding missing properties to chromeos_laptop.c would be my preference. Thanks.
On Fri, Sep 2, 2022 at 11:07 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On Wed, Aug 31, 2022 at 08:17:23PM -0600, Raul Rangel wrote: > > On Wed, Aug 31, 2022 at 1:16 PM Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > > > > > On Wed, Aug 31, 2022 at 12:12:41PM -0700, Dmitry Torokhov wrote: > > > > On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote: > > > > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote: > > > > > > > > > > > > The Elan I2C touchpad driver is currently manually managing the wake > > > > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake > > > > > > and instead relies on the PM subsystem. This is done by calling > > > > > > dev_pm_set_wake_irq. > > > > > > > > > > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device > > > > > > tree, so it's only required when using ACPI. The net result is that this > > > > > > change should be a no-op. i2c_device_remove also already calls > > > > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver. > > > > > > > > > > > > I tested this on an ACPI system where the touchpad doesn't have _PRW > > > > > > defined. I verified I can still wake the system and that the wake source > > > > > > was the touchpad IRQ GPIO. > > > > > > > > > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> > > > > > > > > > > I like this a lot [...] > > > > > > > > > > I also like this a lot, but this assumes that firmware has correct > > > > settings for the interrupt... Unfortunately it is not always the case > > > > and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry, > > > > ect) do not mark interrupt as wakeup: > > > > > > > > src/mainboard/google/glados/variants/chell/overridetree.cb > > > > > > > > chip drivers/i2c/generic > > > > register "hid" = ""ELAN0000"" > > > > register "desc" = ""ELAN Touchpad"" > > > > register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)" > > > > register "wake" = "GPE0_DW0_05" > > > > device i2c 15 on end > > > > > > > > So the above entry specifies the `wake` register. This generates an > > ACPI _PRW resource. The patch series will actually fix devices like > > this. Today without this patch series we get two wake events for a > > device. The ACPI wake GPE specified by the _PRW resource, and the > > erroneous GPIO wake event. But you bring up a good point. > > Does this mean that the example that we currently have in coreboot > documentation (Documentation/acpi/devicetree.md) is not correct: > > device pci 15.0 on > chip drivers/i2c/generic > register "hid" = ""ELAN0000"" > register "desc" = ""ELAN Touchpad"" > register "irq" = "ACPI_IRQ_WAKE_LEVEL_LOW(GPP_A21_IRQ)" > register "wake" = "GPE0_DW0_21" > device i2c 15 on end > end > end # I2C #0 > > Doesn't in say that we have both GpioIrq and GPE wakeup methods defined > for the same device? Hrmm, yeah that is wrong and will cause duplicate wake events for the device. I'll push a CL to clean up the documentation. > > > > > I wrote a quick and dirty script (https://0paste.com/391849) to parse > > the coreboot device tree entries. Open source firmware is great isn't > > it? ;) > > > > $ find src/mainboard/google/ -iname '*.cb' | xargs awk -f touch.awk -- > > src/mainboard/google/eve/devicetree.cb > > ... > > > src/mainboard/google/sarien/variants/sarien/devicetree.cb > > 1 > > chip drivers/i2c/generic > > register "hid" = ""ELAN0000"" > > register "desc" = ""ELAN Touchpad"" > > register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)" > > register "probed" = "1" > > device i2c 2c on end > > end > > Total Touchpad: 202 > > Total Wake: 195 > > > > Out of all the touchpads defined on ChromeOS it looks like only 4 > > devices are missing a wake declaration. I omitted touchpanels because > > ChromeOS doesn't use those as a wake source. chromeos_laptop.c already > > defines some devices with i2c board_info and it sets the > > `I2C_CLIENT_WAKE` flag. I'm not sure if this is actually working as > > expected. `i2c_device_probe` requires a `wakeup` irq to be present in > > the device tree if the `I2C_CLIENT_WAKE` flag is set, but I'm assuming > > No it does not. If there is no wakeup IRQ defined of_irq_get_byname() > will return an error and we'll take the "else if (client->irq > 0)" > branch and will set up client->irq as the wakeup irq. > > > the device tree was missing wake attributes. Oh thanks for pointing that out. I might refactor patch #4 to just set the `I2C_CLIENT_WAKE` flag when `acpi_wake_capable` is true. > > > > > Anyway, patches 6, and 7 are the ones that drop the legacy behavior. I > > can figure out how to add the above boards to chromeos_laptop.c and > > get the wake attribute plumbed, or I can add something directly to the > > elan_i2c_core, etc so others can add overrides for their boards there. > > I'll also send out CLs to fix the device tree configs (not that we > > would run a FW qual just for this change). > > My preference is to limit board-specific hacks in drivers if we can, so > adding missing properties to chromeos_laptop.c would be my preference. How should we handle non chromeos boards? > > Thanks. > > -- > Dmitry Thanks!
Today, i2c drivers are making the assumption that their IRQs can also be used as wake IRQs. This isn't always the case and it can lead to spurious wakes. This has recently started to affect AMD Chromebooks. With the introduction of d62bd5ce12d7 ("pinctrl: amd: Implement irq_set_wake"), the AMD GPIO controller gained the capability to set the wake bit on each GPIO. The ACPI specification defines two ways to inform the system if a device is wake capable: 1) The _PRW object defines the GPE that can be used to wake the system. 2) Setting ExclusiveAndWake or SharedAndWake in the _CRS GpioInt. Currently only the first method is supported. The i2c drivers don't have any indication that the IRQ is wake capable, so they guess. This causes spurious interrupts, for example: * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have `_PRW` or `ExclusiveAndWake` so that means the device can't wake the system. * The IRQ line is active level low for this device and is pulled up by the power resource defined in `_PR0`/`_PR3`. * The i2c driver will (incorrectly) arm the GPIO for wake by calling `enable_irq_wake` as part of its suspend hook. * ACPI will power down the device since it doesn't have a wake GPE associated with it. * When the device is powered down, the IRQ line will drop, and it will trigger a wake event. See the following debug log: [ 42.335804] PM: Suspending system (s2idle) [ 42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable [ 42.467736] power-0416 __acpi_power_off : Power resource [PR00] turned off [ 42.467739] device_pm-0280 device_set_power : Device [H05D] transitioned to D3cold [ 42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd [ 42.535293] PM: Wakeup unrelated to ACPI SCI [ 42.535294] PM: resume from suspend-to-idle In order to fix this, we need to take into account the wake capable bit defined on the GpioInt. This is accomplished by: * Migrating some of the i2c drivers over to using the PM subsystem to manage the wake IRQ. max8925-i2c, elants_i2c, and raydium_i2c_ts still need to be migrated, I can do that depending on the feedback to this patch series. * Expose the wake_capable bit from the ACPI GpioInt resource to the i2c core. * Use the wake_capable bit in the i2c core to call `dev_pm_set_wake_irq`. This reuses the existing device tree flow. * Make the i2c drivers stop calling `dev_pm_set_wake_irq` since it's now handled by the i2c core. * Make the ACPI device PM system aware of the wake_irq. This is necessary so the device doesn't incorrectly get powered down when a wake_irq is enabled. I've tested this code with various combinations of having _PRW, ExclusiveAndWake and power resources all defined or not defined, but it would be great if others could test this out on their hardware. Thanks, Raul Raul E Rangel (8): Input: elan_i2c - Use PM subsystem to manage wake irq HID: i2c-hid: Use PM subsystem to manage wake irq gpiolib: acpi: Add wake_capable parameter to acpi_dev_gpio_irq_get_by i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq HID: i2c-hid: acpi: Stop setting wakeup_capable Input: elan_i2c - Don't set wake_irq when using ACPI HID: i2c-hid: Don't set wake_irq when using ACPI ACPI: PM: Take wake IRQ into consideration when entering suspend-to-idle drivers/acpi/device_pm.c | 19 +++++++++++++++-- drivers/gpio/gpio-pca953x.c | 3 ++- drivers/gpio/gpiolib-acpi.c | 11 +++++++++- drivers/gpio/gpiolib-acpi.h | 2 ++ drivers/hid/i2c-hid/i2c-hid-acpi.c | 5 ----- drivers/hid/i2c-hid/i2c-hid-core.c | 33 +++++++++++------------------ drivers/i2c/i2c-core-acpi.c | 8 +++++-- drivers/i2c/i2c-core-base.c | 17 +++++++++------ drivers/i2c/i2c-core.h | 4 ++-- drivers/input/mouse/elan_i2c_core.c | 14 +++++------- include/linux/acpi.h | 14 +++++++++--- 11 files changed, 78 insertions(+), 52 deletions(-)