* Re: [PATCH v2 1/2] i2c: designware: Fix clock issue when PM is disabled
2025-07-24 4:22 ` [PATCH v2 1/2] i2c: designware: Fix clock issue when PM is disabled Kunihiko Hayashi
@ 2025-07-30 13:31 ` Jarkko Nikula
2025-08-19 22:06 ` Andi Shyti
2025-09-26 7:37 ` Wolfram Sang
2 siblings, 0 replies; 9+ messages in thread
From: Jarkko Nikula @ 2025-07-30 13:31 UTC (permalink / raw)
To: Kunihiko Hayashi, Andy Shevchenko, Mika Westerberg, Jan Dabros,
Andi Shyti
Cc: Kohei Ito, linux-i2c, linux-kernel
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>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/2] i2c: designware: Fix clock issue when PM is disabled
2025-07-24 4:22 ` [PATCH v2 1/2] i2c: designware: Fix clock issue when PM is disabled Kunihiko Hayashi
2025-07-30 13:31 ` Jarkko Nikula
@ 2025-08-19 22:06 ` Andi Shyti
2025-08-20 10:07 ` Kunihiko Hayashi
2025-09-26 7:37 ` Wolfram Sang
2 siblings, 1 reply; 9+ messages in thread
From: Andi Shyti @ 2025-08-19 22:06 UTC (permalink / raw)
To: Kunihiko Hayashi
Cc: Jarkko Nikula, Andy Shevchenko, Mika Westerberg, Jan Dabros,
Kohei Ito, linux-i2c, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/2] i2c: designware: Fix clock issue when PM is disabled
2025-08-19 22:06 ` Andi Shyti
@ 2025-08-20 10:07 ` Kunihiko Hayashi
0 siblings, 0 replies; 9+ messages in thread
From: Kunihiko Hayashi @ 2025-08-20 10:07 UTC (permalink / raw)
To: Andi Shyti
Cc: Jarkko Nikula, Andy Shevchenko, Mika Westerberg, Jan Dabros,
ito.kohei, linux-i2c, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] i2c: designware: Fix clock issue when PM is disabled
2025-07-24 4:22 ` [PATCH v2 1/2] i2c: designware: Fix clock issue when PM is disabled Kunihiko Hayashi
2025-07-30 13:31 ` Jarkko Nikula
2025-08-19 22:06 ` Andi Shyti
@ 2025-09-26 7:37 ` Wolfram Sang
2 siblings, 0 replies; 9+ messages in thread
From: Wolfram Sang @ 2025-09-26 7:37 UTC (permalink / raw)
To: Kunihiko Hayashi
Cc: Jarkko Nikula, Andy Shevchenko, Mika Westerberg, Jan Dabros,
Andi Shyti, Kohei Ito, linux-i2c, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]
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!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread