Bug 202683

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.

(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 |  }

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.

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.

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