On Wed, Feb 8, 2023 at 1:28 AM Amadeusz Sławiński
<amadeuszx.slawinski@xxxxxxxxxxxxxxx> wrote:
>
> On 2/8/2023 7:57 AM, Mika Westerberg wrote:
> > Hi,
> >
> > On Tue, Feb 07, 2023 at 09:33:55AM -0700, Raul Rangel wrote:
> >> Sorry, resending in plain text mode.
> >>
> >> On Tue, Feb 7, 2023 at 12:25 AM Mika Westerberg
> >> <mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> >>>
> >>> After commit b38f2d5d9615 ("i2c: acpi: Use ACPI wake capability bit to
> >>> set wake_irq") the I2C core has been setting I2C_CLIENT_WAKE for ACPI
> >>> devices if they announce to be wake capable in their device description.
> >>> However, on certain systems where audio codec has been connected through
> >>> I2C this causes system suspend to wake up immediately because power to
> >>> the codec is turned off which pulls the interrupt line "low" triggering
> >>> wake up.
> >>>
> >>> Possible reason why the interrupt is marked as wake capable is that some
> >>> codecs apparently support "Wake on Voice" or similar functionality.
> >>
> >> That's generally a bug in the ACPI tables. The wake bit shouldn't be
> >> set if the power domain for the device is powered off on suspend. The
> >> best thing is to fix the ACPI tables, but if you can't, then you can
> >> set the ignore_wake flag for the device:
> >> https://github.com/torvalds/linux/blob/master/drivers/gpio/gpiolib-acpi.c#L31.
> >> If that works we can add a quirk for the device:
> >> https://github.com/torvalds/linux/blob/master/drivers/gpio/gpiolib-acpi.c#L1633.
>
> I've seen this one already and also tried to use it, but it didn't work.
> Also when I was reading code I wasn't really convinced that it is linked
> to i2c in any straightforward way. I mean i2c decides in different
> places that it has wake support (I even added some prints to make sure
> ;). The code you pointed out decides in
> https://github.com/torvalds/linux/blob/master/drivers/gpio/gpiolib-acpi.c#L387
> but i2c code seems to decide in
> https://github.com/torvalds/linux/blob/master/drivers/i2c/i2c-core-acpi.c#L176
> where it just checks if irq flags has wake_capable flag set. When I
> looked at it previously I was pretty sure it comes straight from BIOS
> and passes the quirk code you mentioned, still I may have missed something.
You also need the following patch
https://github.com/torvalds/linux/commit/0e3b175f079247f0d40d2ab695999c309d3a7498,
otherwise the ignore flag only applies to _AEI GPIOs.
>
> >
> > I think (hope) these systems are not yet available for public so there
> > is a chance that the tables can still be fixed, without need to add any
> > quirks.
> >
> > @Amadeusz, @Cezary, if that's the case I suggest filing a bug against
> > the BIOS.
> >
>
> Well, I tried custom DSDT and had problems, but I just remembered that I
> probably need to pass "revision+1" in file, so kernel sees it as a newer
> version, let me try again. Is it enough to replace "ExclusiveAndWake"
> with "Exclusive"?
>
> >>> In any case, I don't think we should be enabling wakeup by default on
> >>> all I2C devices that are wake capable. According to device_init_wakeup()
> >>> documentation most devices should leave it disabled, with exceptions on
> >>> devices such as keyboards, power buttons etc. Userspace can enable
> >>> wakeup as needed by writing to device "power/wakeup" attribute.
> >>
> >> Enabling wake by default was an unintended side-effect. I didn't catch
> >> this when I wrote the patch :/ It's been exposing all the incorrect
> >> ACPI configurations for better or worse. Mario pushed a patch up
> >> earlier to disable thes Wake GPIOs when using S3:
> >> https://github.com/torvalds/linux/commit/d63f11c02b8d3e54bdb65d8c309f73b7f474aec4.
> >> Are you having problems with S3 or S0iX?
> >
> > I think this case is S0ix.
>
> We test both cases in our setups.
IMO if a device needs to support wake from S3 the ACPI table needs to
define a _PRW and define the proper power resources to keep the device
functional during S3.
>
> >
> >>> Reported-by: Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>
> >>> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> >>> ---
> >>> Hi,
> >>>
> >>> Sending this as RFC because I'm not too familiar with the usage of
> >>> I2C_CLIENT_WAKE and whether this is something that is expected behaviour
> >>> in users of I2C devices. On ACPI side I think this is the correct thing
> >>> to do at least.
> >>>
> >>> drivers/i2c/i2c-core-base.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> >>> index 087e480b624c..7046549bdae7 100644
> >>> --- a/drivers/i2c/i2c-core-base.c
> >>> +++ b/drivers/i2c/i2c-core-base.c
> >>> @@ -527,7 +527,7 @@ static int i2c_device_probe(struct device *dev)
> >>> goto put_sync_adapter;
> >>> }
> >>>
> >>> - device_init_wakeup(&client->dev, true);
> >>> + device_init_wakeup(&client->dev, false);
> >>
> >> This would be a change in behavior for Device Tree. Maybe you can
> >> declare a `bool enable_wake = true`, then in the ACPI branch
> >> (https://github.com/torvalds/linux/blob/master/drivers/i2c/i2c-core-base.c#L495)
> >> set `enable_wake = false`. This would keep wakes enabled by default on
> >> device tree and disabled for ACPI. This matches the original behavior
> >> before my patch.
> >
> > I don't think it's a good idea to make the behaviour different. Drivers
> > in general do not need to know whether the device was enumerated on ACPI
> > or DT or whatnot. Same goes for users who should expect similar
> > behaviour on the same device.
> >
> > I wonder what is the reason why I2C bus does this for all wake capable
> > devices in the first place? Typically it should be up to the user to
> > enable them not the opposite.
>
