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

[PATCH v3 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
Fixes: 2d71ee0ce72f ("ACPI / LPSS: Add a device link from the GPU to the BYT I2C5 controller")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Add Fixes: tag

Changes in v3:
-Point Fixes tag to a more apropriate commit
---
 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 v3 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
Fixes: 2d71ee0ce72f ("ACPI / LPSS: Add a device link from the GPU to the BYT I2C5 controller")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Add Fixes: tag

Changes in v3:
-Point Fixes tag to a more apropriate commit
---
 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 v3 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
Fixes: 2d71ee0ce72f ("ACPI / LPSS: Add a device link from the GPU to the BYT I2C5 controller")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Add Fixes: tag

Changes in v3:
-Point Fixes tag to a more apropriate commit
---
 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 v3 3/3] ACPI / LPSS: Add dmi quirk for skipping _DEP check for some device-links

[Rafael J. Wysocki]

On Thu, Oct 24, 2019 at 11:57 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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
> Fixes: 2d71ee0ce72f ("ACPI / LPSS: Add a device link from the GPU to the BYT I2C5 controller")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Add Fixes: tag
>
> Changes in v3:
> -Point Fixes tag to a more apropriate commit
> ---
>  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);
> --

Applying the series as 5.5 material, thanks!

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

[Andy Shevchenko]

On Thu, Oct 24, 2019 at 11:57:21PM +0200, Hans de Goede 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.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Couple of nits below.

> 
> Cc: stable@vger.kernel.org
> Fixes: 2d71ee0ce72f ("ACPI / LPSS: Add a device link from the GPU to the BYT I2C5 controller")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Add Fixes: tag
> 
> Changes in v3:
> -Point Fixes tag to a more apropriate commit
> ---
>  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 */

sdcard -> SD card

>  	{"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) */

non CR -> non-CR

> +	{"80860F41", "7", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
>  };
>  
>  static bool hid_uid_match(struct acpi_device *adev,
> -- 
> 2.23.0
> 

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Oct 24, 2019 at 11:57:22PM +0200, Hans de Goede wrote:
> 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.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Cc: stable@vger.kernel.org
> Fixes: 2d71ee0ce72f ("ACPI / LPSS: Add a device link from the GPU to the BYT I2C5 controller")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Add Fixes: tag
> 
> Changes in v3:
> -Point Fixes tag to a more apropriate commit
> ---
>  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
> 

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Oct 24, 2019 at 11:57:23PM +0200, Hans de Goede wrote:
> 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.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

One comment below.

> Cc: stable@vger.kernel.org
> Fixes: 2d71ee0ce72f ("ACPI / LPSS: Add a device link from the GPU to the BYT I2C5 controller")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Add Fixes: tag
> 
> Changes in v3:
> -Point Fixes tag to a more apropriate commit
> ---
>  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);

Perhaps a helper?

>  
>  	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);

And use it here?

>  
>  	put_device(dev2);
> -- 
> 2.23.0
> 

-- 
With Best Regards,
Andy Shevchenko