[PATCH 1/3] ACPI / LPSS: Add LNXVIDEO -> BYT I2C7 to lpss_device_links

[PATCH 1/3] ACPI / LPSS: Add LNXVIDEO -> BYT I2C7 to lpss_device_links

[Hans de Goede]

So far on Bay Trail (BYT) we only have been adding a device_link adding
the iGPU (LNXVIDEO) device as consumer for the I2C controller for the
PMIC for I2C5, but the PMIC only uses I2C5 on BYT CR (cost reduced) on
regular BYT platforms I2C7 is used and we were not adding the device_link
sometimes causing resume ordering issues.

This commit adds LNXVIDEO -> BYT I2C7 to the lpss_device_links table,
fixing this.

Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpi_lpss.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 60bbc5090abe..e7a4504f0fbf 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -473,9 +473,14 @@ struct lpss_device_links {
  * the supplier is not enumerated until after the consumer is probed.
  */
 static const struct lpss_device_links lpss_device_links[] = {
+	/* CHT External sdcard slot controller depends on PMIC I2C ctrl */
 	{"808622C1", "7", "80860F14", "3", DL_FLAG_PM_RUNTIME},
+	/* CHT iGPU depends on PMIC I2C controller */
 	{"808622C1", "7", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
+	/* BYT CR iGPU depends on PMIC I2C controller (UID 5 on CR) */
 	{"80860F41", "5", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
+	/* BYT iGPU depends on PMIC I2C controller (UID 7 on non CR) */
+	{"80860F41", "7", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
 };
 
 static bool hid_uid_match(struct acpi_device *adev,
-- 
2.23.0

[PATCH 2/3] ACPI / LPSS: Add LNXVIDEO -> BYT I2C1 to lpss_device_links

[Hans de Goede]

Various Asus Bay Trail devices (T100TA, T100CHI, T200TA) have an embedded
controller connected to I2C1 and the iGPU (LNXVIDEO) _PS0/_PS3 methods
access it, so we need to add a consumer link from LNXVIDEO to I2C1 on
these devices to avoid suspend/resume ordering problems.

Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpi_lpss.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index e7a4504f0fbf..cd8cf3333f04 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -477,6 +477,8 @@ static const struct lpss_device_links lpss_device_links[] = {
 	{"808622C1", "7", "80860F14", "3", DL_FLAG_PM_RUNTIME},
 	/* CHT iGPU depends on PMIC I2C controller */
 	{"808622C1", "7", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
+	/* BYT iGPU depends on the Embedded Controller I2C controller (UID 1) */
+	{"80860F41", "1", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
 	/* BYT CR iGPU depends on PMIC I2C controller (UID 5 on CR) */
 	{"80860F41", "5", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
 	/* BYT iGPU depends on PMIC I2C controller (UID 7 on non CR) */
-- 
2.23.0

[PATCH 3/3] ACPI / LPSS: Add dmi quirk for skipping _DEP check for some device-links

[Hans de Goede]

The iGPU / GFX0 device's _PS0 method on the ASUS T200TA depends on the
I2C1 controller (which is connected to the embedded controller). But unlike
in the T100TA/T100CHI this dependency is not listed in the _DEP of the GFX0
device.

This results in the dev_WARN_ONCE(..., "Transfer while suspended\n") call
in i2c-designware-master.c triggering and the AML code not working as it
should.

This commit fixes this by adding a dmi based quirk mechanism for devices
which miss a _DEP, and adding a quirk for the LNXVIDEO depending on the
I2C1 device on the Asus T200TA.

Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpi_lpss.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index cd8cf3333f04..751ed38f2a10 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -10,6 +10,7 @@
 #include <linux/acpi.h>
 #include <linux/clkdev.h>
 #include <linux/clk-provider.h>
+#include <linux/dmi.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/mutex.h>
@@ -463,6 +464,18 @@ struct lpss_device_links {
 	const char *consumer_hid;
 	const char *consumer_uid;
 	u32 flags;
+	const struct dmi_system_id *dep_missing_ids;
+};
+
+/* Please keep this list sorted alphabetically by vendor and model */
+static const struct dmi_system_id i2c1_dep_missing_dmi_ids[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "T200TA"),
+		},
+	},
+	{}
 };
 
 /*
@@ -478,7 +491,8 @@ static const struct lpss_device_links lpss_device_links[] = {
 	/* CHT iGPU depends on PMIC I2C controller */
 	{"808622C1", "7", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
 	/* BYT iGPU depends on the Embedded Controller I2C controller (UID 1) */
-	{"80860F41", "1", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
+	{"80860F41", "1", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME,
+	 i2c1_dep_missing_dmi_ids},
 	/* BYT CR iGPU depends on PMIC I2C controller (UID 5 on CR) */
 	{"80860F41", "5", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
 	/* BYT iGPU depends on PMIC I2C controller (UID 7 on non CR) */
@@ -577,7 +591,8 @@ static void acpi_lpss_link_consumer(struct device *dev1,
 	if (!dev2)
 		return;
 
-	if (acpi_lpss_dep(ACPI_COMPANION(dev2), ACPI_HANDLE(dev1)))
+	if ((link->dep_missing_ids && dmi_check_system(link->dep_missing_ids))
+	    || acpi_lpss_dep(ACPI_COMPANION(dev2), ACPI_HANDLE(dev1)))
 		device_link_add(dev2, dev1, link->flags);
 
 	put_device(dev2);
@@ -592,7 +607,8 @@ static void acpi_lpss_link_supplier(struct device *dev1,
 	if (!dev2)
 		return;
 
-	if (acpi_lpss_dep(ACPI_COMPANION(dev1), ACPI_HANDLE(dev2)))
+	if ((link->dep_missing_ids && dmi_check_system(link->dep_missing_ids))
+	    || acpi_lpss_dep(ACPI_COMPANION(dev1), ACPI_HANDLE(dev2)))
 		device_link_add(dev1, dev2, link->flags);
 
 	put_device(dev2);
-- 
2.23.0

Re: [PATCH 1/3] ACPI / LPSS: Add LNXVIDEO -> BYT I2C7 to lpss_device_links

[Rafael J. Wysocki]

On Thu, Oct 24, 2019 at 11:29 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> So far on Bay Trail (BYT) we only have been adding a device_link adding
> the iGPU (LNXVIDEO) device as consumer for the I2C controller for the
> PMIC for I2C5, but the PMIC only uses I2C5 on BYT CR (cost reduced) on
> regular BYT platforms I2C7 is used and we were not adding the device_link
> sometimes causing resume ordering issues.
>
> This commit adds LNXVIDEO -> BYT I2C7 to the lpss_device_links table,
> fixing this.
>
> Cc: stable@vger.kernel.org

Thanks for these fixes, but it would be kind of nice to have Fixes:
tags for them too.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/acpi_lpss.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index 60bbc5090abe..e7a4504f0fbf 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -473,9 +473,14 @@ struct lpss_device_links {
>   * the supplier is not enumerated until after the consumer is probed.
>   */
>  static const struct lpss_device_links lpss_device_links[] = {
> +       /* CHT External sdcard slot controller depends on PMIC I2C ctrl */
>         {"808622C1", "7", "80860F14", "3", DL_FLAG_PM_RUNTIME},
> +       /* CHT iGPU depends on PMIC I2C controller */
>         {"808622C1", "7", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
> +       /* BYT CR iGPU depends on PMIC I2C controller (UID 5 on CR) */
>         {"80860F41", "5", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
> +       /* BYT iGPU depends on PMIC I2C controller (UID 7 on non CR) */
> +       {"80860F41", "7", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
>  };
>
>  static bool hid_uid_match(struct acpi_device *adev,
> --
> 2.23.0
>

Re: [PATCH 1/3] ACPI / LPSS: Add LNXVIDEO -> BYT I2C7 to lpss_device_links

[Hans de Goede]

Hi,

On 24-10-2019 23:34, Rafael J. Wysocki wrote:
> On Thu, Oct 24, 2019 at 11:29 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> So far on Bay Trail (BYT) we only have been adding a device_link adding
>> the iGPU (LNXVIDEO) device as consumer for the I2C controller for the
>> PMIC for I2C5, but the PMIC only uses I2C5 on BYT CR (cost reduced) on
>> regular BYT platforms I2C7 is used and we were not adding the device_link
>> sometimes causing resume ordering issues.
>>
>> This commit adds LNXVIDEO -> BYT I2C7 to the lpss_device_links table,
>> fixing this.
>>
>> Cc: stable@vger.kernel.org
> 
> Thanks for these fixes, but it would be kind of nice to have Fixes:
> tags for them too.

Ok, v2 with fixes tag coming up.

Regards,

Hans



> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/acpi/acpi_lpss.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>> index 60bbc5090abe..e7a4504f0fbf 100644
>> --- a/drivers/acpi/acpi_lpss.c
>> +++ b/drivers/acpi/acpi_lpss.c
>> @@ -473,9 +473,14 @@ struct lpss_device_links {
>>    * the supplier is not enumerated until after the consumer is probed.
>>    */
>>   static const struct lpss_device_links lpss_device_links[] = {
>> +       /* CHT External sdcard slot controller depends on PMIC I2C ctrl */
>>          {"808622C1", "7", "80860F14", "3", DL_FLAG_PM_RUNTIME},
>> +       /* CHT iGPU depends on PMIC I2C controller */
>>          {"808622C1", "7", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
>> +       /* BYT CR iGPU depends on PMIC I2C controller (UID 5 on CR) */
>>          {"80860F41", "5", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
>> +       /* BYT iGPU depends on PMIC I2C controller (UID 7 on non CR) */
>> +       {"80860F41", "7", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
>>   };
>>
>>   static bool hid_uid_match(struct acpi_device *adev,
>> --
>> 2.23.0
>>

Re: [PATCH 1/3] ACPI / LPSS: Add LNXVIDEO -> BYT I2C7 to lpss_device_links

[Pierre-Louis Bossart]

On 10/24/19 4:34 PM, Rafael J. Wysocki wrote:
> On Thu, Oct 24, 2019 at 11:29 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> So far on Bay Trail (BYT) we only have been adding a device_link adding
>> the iGPU (LNXVIDEO) device as consumer for the I2C controller for the
>> PMIC for I2C5, but the PMIC only uses I2C5 on BYT CR (cost reduced) on
>> regular BYT platforms I2C7 is used and we were not adding the device_link
>> sometimes causing resume ordering issues.
>>
>> This commit adds LNXVIDEO -> BYT I2C7 to the lpss_device_links table,
>> fixing this.
>>
>> Cc: stable@vger.kernel.org
> 
> Thanks for these fixes, but it would be kind of nice to have Fixes:
> tags for them too.

Nice, this removes the warnings I saw on Asus T100TA
[   56.015285] i2c_designware 80860F41:00: Transfer while suspended

Thanks Hans! Feel free to take the following tag for your v2.

Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Maybe an unrelated point, but with this series I now see a new message 
(logged only once):
[   46.888703] ACPI: button: The lid device is not compliant to SW_LID.

Not sure what exactly this is about, but it may be linked to the fact 
that the power button is useless to resume and somehow I have to 
close/reopen the lid to force the device to resume.

if it helps here are the traces for 2 cycles of suspend/resume.

[   34.242313] PM: suspend entry (s2idle)
[   34.246896] Filesystems sync: 0.004 seconds
[   34.247265] Freezing user space processes ... (elapsed 0.001 seconds) 
done.
[   34.249250] OOM killer disabled.
[   34.249253] Freezing remaining freezable tasks ... (elapsed 0.000 
seconds) done.
[   34.250195] printk: Suspending console(s) (use no_console_suspend to 
debug)
[   41.251352] mmc1: queuing unknown CIS tuple 0x80 (2 bytes)
[   41.252948] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
[   41.254530] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
[   41.257397] mmc1: queuing unknown CIS tuple 0x80 (7 bytes)
[   41.586893] OOM killer enabled.
[   41.586898] Restarting tasks ... done.
[   41.625298] video LNXVIDEO:00: Restoring backlight state
[   41.625718] PM: suspend exit
[   45.162584] ax88179_178a 2-1:1.0 enx00051ba24714: ax88179 - Link 
status is: 1
[   45.171220] IPv6: ADDRCONF(NETDEV_CHANGE): enx00051ba24714: link 
becomes ready
[   45.400724] ACPI: button: The lid device is not compliant to SW_LID.
[   58.478184] PM: suspend entry (s2idle)
[   58.528882] Filesystems sync: 0.051 seconds
[   58.529354] Freezing user space processes ... (elapsed 0.004 seconds) 
done.
[   58.533708] OOM killer disabled.
[   58.533712] Freezing remaining freezable tasks ... (elapsed 0.000 
seconds) done.
[   58.534648] printk: Suspending console(s) (use no_console_suspend to 
debug)
[   63.084134] mmc1: queuing unknown CIS tuple 0x80 (2 bytes)
[   63.085736] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
[   63.087337] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
[   63.090241] mmc1: queuing unknown CIS tuple 0x80 (7 bytes)
[   63.420651] OOM killer enabled.
[   63.420656] Restarting tasks ... done.
[   63.458493] video LNXVIDEO:00: Restoring backlight state
[   63.458918] PM: suspend exit
[   66.862343] ax88179_178a 2-1:1.0 enx00051ba24714: ax88179 - Link 
status is: 1
[   66.869564] IPv6: ADDRCONF(NETDEV_CHANGE): enx00051ba24714: link 
becomes ready

Re: [PATCH 1/3] ACPI / LPSS: Add LNXVIDEO -> BYT I2C7 to lpss_device_links

[Hans de Goede]

Hi,

On 25-10-2019 01:18, Pierre-Louis Bossart wrote:
> 
> 
> On 10/24/19 4:34 PM, Rafael J. Wysocki wrote:
>> On Thu, Oct 24, 2019 at 11:29 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> So far on Bay Trail (BYT) we only have been adding a device_link adding
>>> the iGPU (LNXVIDEO) device as consumer for the I2C controller for the
>>> PMIC for I2C5, but the PMIC only uses I2C5 on BYT CR (cost reduced) on
>>> regular BYT platforms I2C7 is used and we were not adding the device_link
>>> sometimes causing resume ordering issues.
>>>
>>> This commit adds LNXVIDEO -> BYT I2C7 to the lpss_device_links table,
>>> fixing this.
>>>
>>> Cc: stable@vger.kernel.org
>>
>> Thanks for these fixes, but it would be kind of nice to have Fixes:
>> tags for them too.
> 
> Nice, this removes the warnings I saw on Asus T100TA
> [   56.015285] i2c_designware 80860F41:00: Transfer while suspended
> 
> Thanks Hans! Feel free to take the following tag for your v2.
> 
> Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Thanks, but I've already send out v2 (without you in the Cc since nothing was changed)
I've added your tested-by to my local version in case another revision is necessary.

> Maybe an unrelated point, but with this series I now see a new message (logged only once):
> [   46.888703] ACPI: button: The lid device is not compliant to SW_LID.

Yes the iGPU _PS0 method seems to much with the LID switch status, but this is
harmless this happens on a lot of laptops. TBH I wonder if we should not just
remove this message?

> Not sure what exactly this is about, but it may be linked to the fact that the power button is useless to resume and somehow I have to close/reopen the lid to force the device to resume.

The powerbutton not working is likely unrelated and TBH is a bit surprising I
do not have a T100TA at hand atm, but on the closely related T200TA it works fine.

I think your kernel .config may have some settings which cause this. Specifically
for the powerbutton to work, it must work as a GPIO-button.

Can you try:

sudo evemu-record

You should then see something like this:

Available devices:
/dev/input/event0:      Lid Switch
/dev/input/event1:      Sleep Button
/dev/input/event2:      Asus Keyboard
/dev/input/event3:      Asus Keyboard
/dev/input/event4:      Asus TouchPad
/dev/input/event5:      SIS0817:00 0457:1084
/dev/input/event6:      Video Bus
/dev/input/event7:      Asus Wireless Radio Control
/dev/input/event8:      Intel HDMI/DP LPE Audio HDMI/DP,pcm=0
/dev/input/event9:      Intel HDMI/DP LPE Audio HDMI/DP,pcm=1
/dev/input/event10:     PC Speaker
/dev/input/event11:     Asus WMI hotkeys
/dev/input/event12:     gpio-keys
/dev/input/event13:     gpio-keys
/dev/input/event14:     bytcr-rt5640 Headset

Then select the second gpio-keys, so in my case I enter: "13<enter>"

Then in the output you should see:

#   Event type 1 (EV_KEY)
#     Event code 116 (KEY_POWER)
#     Event code 125 (KEY_LEFTMETA)
#     Event code 561 ((null))

Among more output, now press the powerbutton, then you should see:

E: 0.000001 0001 0074 0001      # EV_KEY / KEY_POWER            1
E: 0.000001 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +0ms
E: 0.121208 0001 0074 0000      # EV_KEY / KEY_POWER            0
E: 0.121208 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +121ms

After which the machine suspends. Then press it again and the machine should
wakeup (note it does not matter how you put it to sleep) on some devices you
may now see another power-button press for the wake-up. Note note all desktop-
environments handle the second power-button press well. Some immediately
re-suspend again, which may be what you are seeing.

I tried to fix this on the kernel side, but the wakeup press being reported is
a feature Android relies on to no immediately resuspend if woken up another way
so the input kernel folks nacked filtering out the second press. Instead I've
fixed this in userspace for gnome with this commit:

https://gitlab.gnome.org/GNOME/gnome-settings-daemon/commit/f2ae8a3b9905cde7a9c12f78cb84689e97203380

So it could also be that this is what you are seeing. If that is the case
then a single attempt to suspend + resume via power-button + second resume
attempt with the LID should result in 2 suspends/resumes showing in dmesg
and you should see they wakeup-powerbutton press in evemu-record.

One possible fix for this would be to switch your DE to a recent GNOME.

Regards,

Hans








> 
> if it helps here are the traces for 2 cycles of suspend/resume.
> 
> [   34.242313] PM: suspend entry (s2idle)
> [   34.246896] Filesystems sync: 0.004 seconds
> [   34.247265] Freezing user space processes ... (elapsed 0.001 seconds) done.
> [   34.249250] OOM killer disabled.
> [   34.249253] Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
> [   34.250195] printk: Suspending console(s) (use no_console_suspend to debug)
> [   41.251352] mmc1: queuing unknown CIS tuple 0x80 (2 bytes)
> [   41.252948] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> [   41.254530] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> [   41.257397] mmc1: queuing unknown CIS tuple 0x80 (7 bytes)
> [   41.586893] OOM killer enabled.
> [   41.586898] Restarting tasks ... done.
> [   41.625298] video LNXVIDEO:00: Restoring backlight state
> [   41.625718] PM: suspend exit
> [   45.162584] ax88179_178a 2-1:1.0 enx00051ba24714: ax88179 - Link status is: 1
> [   45.171220] IPv6: ADDRCONF(NETDEV_CHANGE): enx00051ba24714: link becomes ready
> [   45.400724] ACPI: button: The lid device is not compliant to SW_LID.
> [   58.478184] PM: suspend entry (s2idle)
> [   58.528882] Filesystems sync: 0.051 seconds
> [   58.529354] Freezing user space processes ... (elapsed 0.004 seconds) done.
> [   58.533708] OOM killer disabled.
> [   58.533712] Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
> [   58.534648] printk: Suspending console(s) (use no_console_suspend to debug)
> [   63.084134] mmc1: queuing unknown CIS tuple 0x80 (2 bytes)
> [   63.085736] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> [   63.087337] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> [   63.090241] mmc1: queuing unknown CIS tuple 0x80 (7 bytes)
> [   63.420651] OOM killer enabled.
> [   63.420656] Restarting tasks ... done.
> [   63.458493] video LNXVIDEO:00: Restoring backlight state
> [   63.458918] PM: suspend exit
> [   66.862343] ax88179_178a 2-1:1.0 enx00051ba24714: ax88179 - Link status is: 1
> [   66.869564] IPv6: ADDRCONF(NETDEV_CHANGE): enx00051ba24714: link becomes ready
>