Bug 202683 - Runtime suspended intel-lpss takes long time to resume
Summary: Runtime suspended intel-lpss takes long time to resume
Status: RESOLVED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: I2C (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Drivers/I2C virtual user
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-02-26 07:25 UTC by Kai-Heng Feng
Modified: 2019-09-18 07:56 UTC (History)
4 users (show)

See Also:
Kernel Version: mainline
Tree: Mainline
Regression: No


Attachments

Description Kai-Heng Feng 2019-02-26 07:25:52 UTC
Goodix i2c-hid touchpad may miss its first IRQ because i2c-designware-platdrv takes a long time in its runtime resume callback, mostly i2c_dw_init_master().
Disable runtime PM on it can workaround the issue.

I am wondering the true benefit of runtime PM for i2c-designware-platdrv? From what I can measure, it saves little to none power.

The runtime PM supports is added by this commit:
1fc2fe204cb9d8010e70716aa48b5c3db6379da3
Author: Mika Westerberg <mika.westerberg@linux.intel.com>
Date:   Thu May 15 17:37:23 2014 +0300

    i2c: designware: Add runtime PM hooks
    
    It is possible that after entering runtime PM suspend the controller
    context is lost due the fact that its power is removed. This happens for
    example on Asus T100, an Intel Baytrail based tablet/laptop.
    
    In order to get the controller back to functional state, we need to
    implement runtime PM hooks which will re-initialize the hardware during
    runtime PM resume. We can re-use the existing system suspend hooks as the
    steps to resume/suspend the controller are the same.
    
    Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
    Signed-off-by: Wolfram Sang <wsa@the-dreams.de>

According to the commit log, a system resume callback should be suffice for the issue?
Comment 1 Jarkko Nikula 2019-02-28 09:23:25 UTC
Benefit of runtime PM here is that when I2C controller is in D3 low-power state it doesn't block the system from reaching the S0ix-states.

As far as I know the S0ix power consumption is near to S3 system suspend power consumption so there is a great benefit by letting all devices to be in D3 state when they are idle since otherwise system would stay in S0 consuming much more power. I think in S0 state difference between I2C controller being in D0 or D3 is negligible as you have measured. But huge if I2C would prevent the system from entering S0ix.

I think we should find why resume takes so long.
Comment 2 Kai-Heng Feng 2019-03-06 08:36:22 UTC
(In reply to Jarkko Nikula from comment #1)
> Benefit of runtime PM here is that when I2C controller is in D3 low-power
> state it doesn't block the system from reaching the S0ix-states.
> 

I can see the SoC still enters PC10 after disabling runtime suspend of the i2c-dw-plat driver.

> As far as I know the S0ix power consumption is near to S3 system suspend
> power consumption so there is a great benefit by letting all devices to be
> in D3 state when they are idle since otherwise system would stay in S0
> consuming much more power. I think in S0 state difference between I2C
> controller being in D0 or D3 is negligible as you have measured. But huge if
> I2C would prevent the system from entering S0ix.

Although the system can enter PC10, low_power_idle_system_residency_us is always 0 when i2c-dw-plat is runtime suspended.
Not sure which one is real indicator for S0ix though.

> 
> I think we should find why resume takes so long.
Here's the ftrace:
# tracer: function_graph
#
# CPU  DURATION                  FUNCTION CALLS
# |     |   |                     |   |   |   |
 0)               |  i2c_dw_xfer() {
 0)   6.372 us    |    i2c_dw_prepare_clk();
 0)               |    i2c_dw_init_master() {
 0)   0.511 us    |      i2c_dw_acquire_lock();
 0)   6.289 us    |      __i2c_dw_disable();
 0)   1.258 us    |      i2c_dw_release_lock();
 0) + 10.043 us   |    }
 0)   0.343 us    |    i2c_dw_acquire_lock();
 0)   4.400 us    |    i2c_dw_wait_bus_not_busy();
 0)   3.401 us    |    __i2c_dw_disable();
 0)   1.580 us    |    i2c_dw_disable_int();
 2) + 11.179 us   |  i2c_dw_isr();
 2) + 12.145 us   |  i2c_dw_isr();
 2) + 12.763 us   |  i2c_dw_isr();
 2) + 11.732 us   |  i2c_dw_isr();
 2) + 14.130 us   |  i2c_dw_isr();
 0)   1.042 us    |    i2c_dw_release_lock();
 0) @ 109308.8 us |  }
Comment 3 Kai-Heng Feng 2019-03-06 08:46:23 UTC
This can be a hardware design issue from the touchpad IC (made by Goodix):

From the HID over I2C spec:
It is the responsibility of the DEVICE to assert the interrupt until all the data has been read for that specific report. After reading the Input Report, the DEVICE can continue to or reassert the interrupt if there are additional Input Report(s) to be retrieved from the DEVICE.

For this case, the Input Report was discarded by the DEVICE when i2c-dw-plat is runtime resuming.

Closing this as this is quite likely a hardware issue.
Comment 4 Kai-Heng Feng 2019-07-05 04:22:18 UTC
The Goodix touchpad in question has rather small buffer to store 13 input events, and it drops some of its data if the HOST doesn't read them in time.
Comment 5 Kai-Heng Feng 2019-09-18 07:56:15 UTC
Fixed by
commit 76380a607ba0b28627c9b4b55cd47a079a59624b
Author: Kai-Heng Feng <kai.heng.feng@canonical.com>
Date:   Fri Jul 5 12:55:03 2019 +0800

    mfd: intel-lpss: Remove D3cold delay

Note You need to log in before you can comment on or make changes to this bug.