[v2] i2c: designware: Fixes clock issues with platform driver

[PATCH v2 1/2] i2c: designware: Fix clock issue when PM is disabled

Posted by Kunihiko Hayashi 2 months, 2 weeks ago

When removing the driver, enable the clocks once by calling
pm_runtime_get_sync(), and call pm_runtime_put_sync() to disable
the clocks.

If CONFIG_PM=y, clocks for this controller are disabled when it's in
the idle state. So the clocks are properly disabled when the driver
exits.

Othewise, the clocks are always enabled and the PM functions have
no effect. Therefore, the driver exits without disabling the clocks.

    # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
    18
    # echo 1214a000.i2c > /sys/bus/platform/drivers/i2c_designware/bind
    # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
    20
    # echo 1214a000.i2c > /sys/bus/platform/drivers/i2c_designware/unbind
    # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
    20

To ensure that the clocks can be disabled correctly even without
CONFIG_PM=y, should add the following fixes:

- Replace with pm_runtime_put_noidle(), which only decrements the runtime
  PM usage count.
- Call i2c_dw_prepare_clk(false) to explicitly disable the clocks.

Fixes: 7272194ed391f ("i2c-designware: add minimal support for runtime PM")
Co-developed-by: Kohei Ito <ito.kohei@socionext.com>
Signed-off-by: Kohei Ito <ito.kohei@socionext.com>
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d6e1ee935399..edaebfb165f9 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -325,9 +325,11 @@ static void dw_i2c_plat_remove(struct platform_device *pdev)
 	i2c_dw_disable(dev);
 
 	pm_runtime_dont_use_autosuspend(device);
-	pm_runtime_put_sync(device);
+	pm_runtime_put_noidle(device);
 	dw_i2c_plat_pm_cleanup(dev);
 
+	i2c_dw_prepare_clk(dev, false);
+
 	i2c_dw_remove_lock_support(dev);
 
 	reset_control_assert(dev->rst);
-- 
2.34.1

Re: [PATCH v2 1/2] i2c: designware: Fix clock issue when PM is disabled

Posted by Wolfram Sang 1 week, 2 days ago

On Thu, Jul 24, 2025 at 01:22:10PM +0900, Kunihiko Hayashi wrote:
> When removing the driver, enable the clocks once by calling
> pm_runtime_get_sync(), and call pm_runtime_put_sync() to disable
> the clocks.
> 
> If CONFIG_PM=y, clocks for this controller are disabled when it's in
> the idle state. So the clocks are properly disabled when the driver
> exits.
> 
> Othewise, the clocks are always enabled and the PM functions have
> no effect. Therefore, the driver exits without disabling the clocks.
> 
>     # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>     18
>     # echo 1214a000.i2c > /sys/bus/platform/drivers/i2c_designware/bind
>     # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>     20
>     # echo 1214a000.i2c > /sys/bus/platform/drivers/i2c_designware/unbind
>     # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>     20
> 
> To ensure that the clocks can be disabled correctly even without
> CONFIG_PM=y, should add the following fixes:
> 
> - Replace with pm_runtime_put_noidle(), which only decrements the runtime
>   PM usage count.
> - Call i2c_dw_prepare_clk(false) to explicitly disable the clocks.
> 
> Fixes: 7272194ed391f ("i2c-designware: add minimal support for runtime PM")
> Co-developed-by: Kohei Ito <ito.kohei@socionext.com>
> Signed-off-by: Kohei Ito <ito.kohei@socionext.com>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Applied to for-next with the commit message from Andi, thanks!

Re: [PATCH v2 1/2] i2c: designware: Fix clock issue when PM is disabled

Posted by Andi Shyti 1 month, 2 weeks ago

Hi Kunihiko,

On Thu, Jul 24, 2025 at 01:22:10PM +0900, Kunihiko Hayashi wrote:
> When removing the driver, enable the clocks once by calling
> pm_runtime_get_sync(), and call pm_runtime_put_sync() to disable
> the clocks.

can we rephrase this to something like:

  When the driver is removed, the clocks are first enabled by
  calling pm_runtime_get_sync(), and then disabled with
  pm_runtime_put_sync().

Does it work?

> If CONFIG_PM=y, clocks for this controller are disabled when it's in
> the idle state. So the clocks are properly disabled when the driver
> exits.
> 
> Othewise, the clocks are always enabled and the PM functions have
> no effect. Therefore, the driver exits without disabling the clocks.
> 
>     # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>     18
>     # echo 1214a000.i2c > /sys/bus/platform/drivers/i2c_designware/bind
>     # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>     20
>     # echo 1214a000.i2c > /sys/bus/platform/drivers/i2c_designware/unbind
>     # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>     20
> 
> To ensure that the clocks can be disabled correctly even without
> CONFIG_PM=y, should add the following fixes:
> 
> - Replace with pm_runtime_put_noidle(), which only decrements the runtime
>   PM usage count.
> - Call i2c_dw_prepare_clk(false) to explicitly disable the clocks.
> 
> Fixes: 7272194ed391f ("i2c-designware: add minimal support for runtime PM")

This commit doesn't look quite right to me, although it's quite
difficult to find the culprit, as the clk api's have changed a
lot over time.

Do you think this is better:

Fixes: b33af11de236 ("i2c: designware: Do not require clock when SSCN and FFCN are provided")

It doesn't matter much as it won't apply in any of the two
versions.

The Fixes tag should be added in both the patches, but there is
no need to resend, I will apply them once we agree on the commit
message.

Andi

Re: [PATCH v2 1/2] i2c: designware: Fix clock issue when PM is disabled

Posted by Kunihiko Hayashi 1 month, 2 weeks ago

Hi Andi,

Thank you for your comment.

On 2025/08/20 7:06, Andi Shyti wrote:
> Hi Kunihiko,
> 
> On Thu, Jul 24, 2025 at 01:22:10PM +0900, Kunihiko Hayashi wrote:
>> When removing the driver, enable the clocks once by calling
>> pm_runtime_get_sync(), and call pm_runtime_put_sync() to disable
>> the clocks.
> 
> can we rephrase this to something like:
> 
>    When the driver is removed, the clocks are first enabled by
>    calling pm_runtime_get_sync(), and then disabled with
>    pm_runtime_put_sync().
> 
> Does it work?

Yes, your description is better.

> 
>> If CONFIG_PM=y, clocks for this controller are disabled when it's in
>> the idle state. So the clocks are properly disabled when the driver
>> exits.
>>
>> Othewise, the clocks are always enabled and the PM functions have
>> no effect. Therefore, the driver exits without disabling the clocks.
>>
>>      # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>>      18
>>      # echo 1214a000.i2c > /sys/bus/platform/drivers/i2c_designware/bind
>>      # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>>      20
>>      # echo 1214a000.i2c >
> /sys/bus/platform/drivers/i2c_designware/unbind
>>      # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>>      20
>>
>> To ensure that the clocks can be disabled correctly even without
>> CONFIG_PM=y, should add the following fixes:
>>
>> - Replace with pm_runtime_put_noidle(), which only decrements the
> runtime
>>    PM usage count.
>> - Call i2c_dw_prepare_clk(false) to explicitly disable the clocks.
>>
>> Fixes: 7272194ed391f ("i2c-designware: add minimal support for runtime
> PM")
> 
> This commit doesn't look quite right to me, although it's quite
> difficult to find the culprit, as the clk api's have changed a
> lot over time.
> 
> Do you think this is better:
> 
> Fixes: b33af11de236 ("i2c: designware: Do not require clock when SSCN and
> FFCN are provided")

Surely, the code that handles PM and clocks has changed some time,
so it's difficult to identify the commit with "Fixes" tag.
I think the first commit that called pm_runtime_put_sync() was the cause.

> It doesn't matter much as it won't apply in any of the two
> versions> 
> The Fixes tag should be added in both the patches, but there is
> no need to resend, I will apply them once we agree on the commit
> message.

Okay, I agree with you.

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH v2 1/2] i2c: designware: Fix clock issue when PM is disabled

Posted by Jarkko Nikula 2 months, 1 week ago

Hi

On 7/24/25 7:22 AM, Kunihiko Hayashi wrote:
> When removing the driver, enable the clocks once by calling
> pm_runtime_get_sync(), and call pm_runtime_put_sync() to disable
> the clocks.
> 
> If CONFIG_PM=y, clocks for this controller are disabled when it's in
> the idle state. So the clocks are properly disabled when the driver
> exits.
> 
> Othewise, the clocks are always enabled and the PM functions have
> no effect. Therefore, the driver exits without disabling the clocks.
> 
>      # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>      18
>      # echo 1214a000.i2c > /sys/bus/platform/drivers/i2c_designware/bind
>      # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>      20
>      # echo 1214a000.i2c > /sys/bus/platform/drivers/i2c_designware/unbind
>      # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>      20
> 
> To ensure that the clocks can be disabled correctly even without
> CONFIG_PM=y, should add the following fixes:
> 
> - Replace with pm_runtime_put_noidle(), which only decrements the runtime
>    PM usage count.
> - Call i2c_dw_prepare_clk(false) to explicitly disable the clocks.
> 
> Fixes: 7272194ed391f ("i2c-designware: add minimal support for runtime PM")
> Co-developed-by: Kohei Ito <ito.kohei@socionext.com>
> Signed-off-by: Kohei Ito <ito.kohei@socionext.com>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>   drivers/i2c/busses/i2c-designware-platdrv.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
Good catch!

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>