[PATCH] i2c: designware: Add disable runtime pm quirk

[PATCH] i2c: designware: Add disable runtime pm quirk

[AceLan Kao]

Dell machines come with goodix touchpad IC suffer from the double click
issue if the Designware I2C adapter enters runtime suspend.

It's because the goodix re-assert the interrupt if host doesn't read the
data within 100ms and designware takes a longer time to wake up from
runtime suspend. In the case, it got a second interrupt during
resuming, so it thinks it's a double click.

There is no simple way to fix this, it's a firmware issue and goodix
agrees to fix this in their firmware on next release, but this issue
still affects the machines that don't come with an updated firmware. So,
add a quirk to mark those machines and avoid the designware from
entering runtime suspend.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=202683

Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
 drivers/i2c/busses/i2c-designware-master.c | 30 ++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index d464799e40a3..4048a66355f6 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -9,6 +9,7 @@
  * Copyright (C) 2009 Provigent Ltd.
  */
 #include <linux/delay.h>
+#include <linux/dmi.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/export.h>
@@ -22,6 +23,25 @@
 
 #include "i2c-designware-core.h"
 
+static int no_runtime_pm;
+static const struct dmi_system_id i2c_dw_no_runtime_pm[] = {
+	{
+		.ident = "Dell Inspiron 5390",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Inspiron 5390"),
+		},
+	},
+	{
+		.ident = "Dell Vostro 5390",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5390"),
+		},
+	},
+	{ }
+};
+
 static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
 {
 	/* Configure Tx/Rx FIFO threshold levels */
@@ -424,7 +444,8 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
 
-	pm_runtime_get_sync(dev->dev);
+	if (!no_runtime_pm)
+		pm_runtime_get_sync(dev->dev);
 
 	if (dev_WARN_ONCE(dev->dev, dev->suspended, "Transfer while suspended\n")) {
 		ret = -ESHUTDOWN;
@@ -501,7 +522,8 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 done_nolock:
 	pm_runtime_mark_last_busy(dev->dev);
-	pm_runtime_put_autosuspend(dev->dev);
+	if (!no_runtime_pm)
+		pm_runtime_put_autosuspend(dev->dev);
 
 	return ret;
 }
@@ -733,6 +755,10 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
 	if (ret)
 		return ret;
 
+	no_runtime_pm = dmi_check_system(i2c_dw_no_runtime_pm);
+	if (no_runtime_pm)
+		__pm_runtime_disable(dev->dev, true);
+
 	/*
 	 * Increment PM usage count during adapter registration in order to
 	 * avoid possible spurious runtime suspend when adapter device is
-- 
2.17.1

Re: [PATCH] i2c: designware: Add disable runtime pm quirk

[Jarkko Nikula]

On 6/25/19 11:30 AM, AceLan Kao wrote:
> Dell machines come with goodix touchpad IC suffer from the double click
> issue if the Designware I2C adapter enters runtime suspend.
> 
> It's because the goodix re-assert the interrupt if host doesn't read the
> data within 100ms and designware takes a longer time to wake up from
> runtime suspend. In the case, it got a second interrupt during
> resuming, so it thinks it's a double click.
> 
> There is no simple way to fix this, it's a firmware issue and goodix
> agrees to fix this in their firmware on next release, but this issue
> still affects the machines that don't come with an updated firmware. So,
> add a quirk to mark those machines and avoid the designware from
> entering runtime suspend.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=202683
> 
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> ---
>   drivers/i2c/busses/i2c-designware-master.c | 30 ++++++++++++++++++++--
>   1 file changed, 28 insertions(+), 2 deletions(-)
> 
I think better place to have this fixed is in 
drivers/hid/i2c-hid/i2c-hid-core.c by forcing the adapter device active 
when communicating with such touchpad.

In that way only bus where touchpad is connected stays active, not all 
and makes sure issue is handled also if that touchpad is ever connected 
to any other I2C adapter than Designware.

I did something similar in the commit 72bfcee11cf8 ("i2c: Prevent 
runtime suspend of adapter when Host Notify is required"). Not exactly 
same issue but similar idea.

By looking at i2c-hid-core.c I saw a few i2c-hid devices have 
I2C_HID_QUIRK_NO_RUNTIME_PM. Could you test how does this Goodix behave 
if only i2c-hid device runtime PM is prevented not I2C adapter?

A very quick test would be to comment out those lines below:

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c 
b/drivers/hid/i2c-hid/i2c-hid-core.c
index 90164fed08d3..bd3e6570c45e 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -1156,8 +1156,8 @@ static int i2c_hid_probe(struct i2c_client *client,
  		goto err_mem_free;
  	}

-	if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
-		pm_runtime_put(&client->dev);
+//	if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
+//		pm_runtime_put(&client->dev);

  	return 0;

@@ -1183,8 +1183,8 @@ static int i2c_hid_remove(struct i2c_client *client)
  	struct i2c_hid *ihid = i2c_get_clientdata(client);
  	struct hid_device *hid;

-	if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
-		pm_runtime_get_sync(&client->dev);
+//	if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
+//		pm_runtime_get_sync(&client->dev);
  	pm_runtime_disable(&client->dev);
  	pm_runtime_set_suspended(&client->dev);
  	pm_runtime_put_noidle(&client->dev);

-- 
Jarkko

Re: [PATCH] i2c: designware: Add disable runtime pm quirk

[AceLan Kao]

Adding I2C_HID_QUIRK_NO_RUNTIME_PM quirk doesn't help on this issue.
Actually, Goodix touchpad already has that PM quirk in the list for other issue.
        { I2C_VENDOR_ID_GOODIX, I2C_DEVICE_ID_GOODIX_01F0,
               I2C_HID_QUIRK_NO_RUNTIME_PM },
I also modify the code as you suggested, but no luck.

It's not Goodix takes time to wakeup, it's designware I2C controller.
Designware doesn't do anything wrong here, it's Goodix set the interrupt timeout
that leads to the issue, so we have to prevent designware from runtime
suspended.

Jarkko Nikula <jarkko.nikula@linux.intel.com> 於 2019年6月25日 週二 下午9:38寫道：
>
> On 6/25/19 11:30 AM, AceLan Kao wrote:
> > Dell machines come with goodix touchpad IC suffer from the double click
> > issue if the Designware I2C adapter enters runtime suspend.
> >
> > It's because the goodix re-assert the interrupt if host doesn't read the
> > data within 100ms and designware takes a longer time to wake up from
> > runtime suspend. In the case, it got a second interrupt during
> > resuming, so it thinks it's a double click.
> >
> > There is no simple way to fix this, it's a firmware issue and goodix
> > agrees to fix this in their firmware on next release, but this issue
> > still affects the machines that don't come with an updated firmware. So,
> > add a quirk to mark those machines and avoid the designware from
> > entering runtime suspend.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=202683
> >
> > Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> > ---
> >   drivers/i2c/busses/i2c-designware-master.c | 30 ++++++++++++++++++++--
> >   1 file changed, 28 insertions(+), 2 deletions(-)
> >
> I think better place to have this fixed is in
> drivers/hid/i2c-hid/i2c-hid-core.c by forcing the adapter device active
> when communicating with such touchpad.
>
> In that way only bus where touchpad is connected stays active, not all
> and makes sure issue is handled also if that touchpad is ever connected
> to any other I2C adapter than Designware.
>
> I did something similar in the commit 72bfcee11cf8 ("i2c: Prevent
> runtime suspend of adapter when Host Notify is required"). Not exactly
> same issue but similar idea.
>
> By looking at i2c-hid-core.c I saw a few i2c-hid devices have
> I2C_HID_QUIRK_NO_RUNTIME_PM. Could you test how does this Goodix behave
> if only i2c-hid device runtime PM is prevented not I2C adapter?
>
> A very quick test would be to comment out those lines below:
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c
> b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 90164fed08d3..bd3e6570c45e 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -1156,8 +1156,8 @@ static int i2c_hid_probe(struct i2c_client *client,
>                 goto err_mem_free;
>         }
>
> -       if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
> -               pm_runtime_put(&client->dev);
> +//     if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
> +//             pm_runtime_put(&client->dev);
>
>         return 0;
>
> @@ -1183,8 +1183,8 @@ static int i2c_hid_remove(struct i2c_client *client)
>         struct i2c_hid *ihid = i2c_get_clientdata(client);
>         struct hid_device *hid;
>
> -       if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
> -               pm_runtime_get_sync(&client->dev);
> +//     if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
> +//             pm_runtime_get_sync(&client->dev);
>         pm_runtime_disable(&client->dev);
>         pm_runtime_set_suspended(&client->dev);
>         pm_runtime_put_noidle(&client->dev);
>
> --
> Jarkko

Re: [PATCH] i2c: designware: Add disable runtime pm quirk

[Jarkko Nikula]

On 6/26/19 5:32 AM, AceLan Kao wrote:
> Adding I2C_HID_QUIRK_NO_RUNTIME_PM quirk doesn't help on this issue.
> Actually, Goodix touchpad already has that PM quirk in the list for other issue.
>          { I2C_VENDOR_ID_GOODIX, I2C_DEVICE_ID_GOODIX_01F0,
>                 I2C_HID_QUIRK_NO_RUNTIME_PM },
> I also modify the code as you suggested, but no luck.
> 
Yeah, I realized it won't help as the i2c-hid device is anyway powered 
on constantly when the device is open by the pm_runtime_get_sync() call 
in i2c_hid_open().

> It's not Goodix takes time to wakeup, it's designware I2C controller.
> Designware doesn't do anything wrong here, it's Goodix set the interrupt timeout
> that leads to the issue, so we have to prevent designware from runtime
> suspended.
> But only on that bus where Goodix is connected and open by user space. 
What I mean something like below:

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c 
b/drivers/hid/i2c-hid/i2c-hid-core.c
index 90164fed08d3..bbeaa39ddc23 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -795,6 +795,9 @@ static int i2c_hid_open(struct hid_device *hid)
  	struct i2c_hid *ihid = i2c_get_clientdata(client);
  	int ret = 0;

+	/* some quirk test here */
+	pm_runtime_get_sync(&client->adapter->dev);
+
  	ret = pm_runtime_get_sync(&client->dev);
  	if (ret < 0)
  		return ret;
@@ -812,6 +815,9 @@ static void i2c_hid_close(struct hid_device *hid)

  	/* Save some power */
  	pm_runtime_put(&client->dev);
+
+	/* some quirk test here */
+	pm_runtime_put(&client->adapter->dev);
  }

  static int i2c_hid_power(struct hid_device *hid, int lvl)

-- 
Jarkko

Re: [PATCH] i2c: designware: Add disable runtime pm quirk

[AceLan Kao]

Hi Jarkko,

Sorry, I lost track of this thread and didn't understand what you want
me to try.
I'm willing to try it if you can explain it more.

My colleague comes out another solution for this issue
   https://lkml.org/lkml/2019/7/5/17
and it explains why it takes up to 100ms to wake up.
This solution is more aggressive to zero the d3cold_delay and it looks
like the delay is not necessary.

Anyway, we have two different proposed solutions now, my proposed
solution is specific to the listed platforms, but we may have to
extent the list(platforms which uses the old firmware),
the other proposed solution from my colleague is generic and apply to
all platforms which loads intel-lpss-pci driver, it may lead to
unexpected regressions which we don't see now.
Please give some suggestions, thanks.

Best regards,
AceLan Kao.

Jarkko Nikula <jarkko.nikula@linux.intel.com> 於 2019年6月26日 週三 下午2:27寫道：
>
> On 6/26/19 5:32 AM, AceLan Kao wrote:
> > Adding I2C_HID_QUIRK_NO_RUNTIME_PM quirk doesn't help on this issue.
> > Actually, Goodix touchpad already has that PM quirk in the list for other issue.
> >          { I2C_VENDOR_ID_GOODIX, I2C_DEVICE_ID_GOODIX_01F0,
> >                 I2C_HID_QUIRK_NO_RUNTIME_PM },
> > I also modify the code as you suggested, but no luck.
> >
> Yeah, I realized it won't help as the i2c-hid device is anyway powered
> on constantly when the device is open by the pm_runtime_get_sync() call
> in i2c_hid_open().
>
> > It's not Goodix takes time to wakeup, it's designware I2C controller.
> > Designware doesn't do anything wrong here, it's Goodix set the interrupt timeout
> > that leads to the issue, so we have to prevent designware from runtime
> > suspended.
> > But only on that bus where Goodix is connected and open by user space.
> What I mean something like below:
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c
> b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 90164fed08d3..bbeaa39ddc23 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -795,6 +795,9 @@ static int i2c_hid_open(struct hid_device *hid)
>         struct i2c_hid *ihid = i2c_get_clientdata(client);
>         int ret = 0;
>
> +       /* some quirk test here */
> +       pm_runtime_get_sync(&client->adapter->dev);
> +
>         ret = pm_runtime_get_sync(&client->dev);
>         if (ret < 0)
>                 return ret;
> @@ -812,6 +815,9 @@ static void i2c_hid_close(struct hid_device *hid)
>
>         /* Save some power */
>         pm_runtime_put(&client->dev);
> +
> +       /* some quirk test here */
> +       pm_runtime_put(&client->adapter->dev);
>   }
>
>   static int i2c_hid_power(struct hid_device *hid, int lvl)
>
> --
> Jarkko