diff mbox series

i2c: imx-lpi2c: Fix clock count when probe defers

Message ID 20250421062341.2471922-1-carlos.song@nxp.com
State New
Headers show
Series i2c: imx-lpi2c: Fix clock count when probe defers | expand

Commit Message

Carlos Song April 21, 2025, 6:23 a.m. UTC
From: Clark Wang <xiaoning.wang@nxp.com>

Deferred probe with pm_runtime_put() may delay clock disable, causing
incorrect clock usage count. Use pm_runtime_put_sync() to ensure the
clock is disabled immediately.

Signed-off-by: Carlos Song <carlos.song@nxp.com>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Signed-off-by: Jun Li <jun.li@nxp.com>
Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Andi Shyti April 25, 2025, 11:29 a.m. UTC | #1
On Mon, Apr 21, 2025 at 02:23:41PM +0800, carlos.song@nxp.com wrote:
> From: Clark Wang <xiaoning.wang@nxp.com>
> 
> Deferred probe with pm_runtime_put() may delay clock disable, causing
> incorrect clock usage count. Use pm_runtime_put_sync() to ensure the
> clock is disabled immediately.

What is the outcome of the incorrect clock usage count? Do you
have any logs?

> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> Signed-off-by: Jun Li <jun.li@nxp.com>
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>

Carlo's SoB should be at the end of the chain. Should be nice to
know what these are, though, are they co-developed-by? tested-by?
Why so many SoB's?

(no need to resend for this)

Andi
Carlos Song April 27, 2025, 7:23 a.m. UTC | #2
> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Friday, April 25, 2025 7:29 PM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: Aisheng Dong <aisheng.dong@nxp.com>; Frank Li <frank.li@nxp.com>;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; linux-i2c@vger.kernel.org; imx@lists.linux.dev;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH] i2c: imx-lpi2c: Fix clock count when probe defers
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
> 
> 
> On Mon, Apr 21, 2025 at 02:23:41PM +0800, carlos.song@nxp.com wrote:
> > From: Clark Wang <xiaoning.wang@nxp.com>
> >
> > Deferred probe with pm_runtime_put() may delay clock disable, causing
> > incorrect clock usage count. Use pm_runtime_put_sync() to ensure the
> > clock is disabled immediately.
> 
> What is the outcome of the incorrect clock usage count? Do you have any logs?
> 

Hi, Andi

Thank you!

At imx943 board, I notice sometimes after boot one of lpi2cs enable count is 1, but now the controller has been suspend(enable count should be 0).

This is a lpi2c4 example:

root@imx943evk:~# cat /sys/kernel/debug/clk/clk_summary
                                 enable  prepare  protect                                duty  hardware                            connection
   clock                          count    count    count        rate   accuracy phase  cycle    enable   consumer                         id

    lpi2c8                           0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
    lpi2c7                           0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
    lpi2c6                           0       2        1        24000000    0          0     50000      Y      i2c@426c0000                    no_connection_id
    lpi2c5                           0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
-->  lpi2c4                           1       2        1        24000000    0          0     50000      Y      i2c@42540000                    no_connection_id
    lpi2c3                           0       2        1        24000000    0          0     50000      Y      i2c@42530000                    no_connection_id
    lpi2c2                           0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
    lpi2c1                           0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id

root@imx943evk:~# cat /sys/devices/platform/soc/42000000.bus/42540000.i2c/power/runtime_status
suspended

Normal case should be this:
Enable count is 0 and controller is suspended.

root@imx943evk:~#  cat /sys/kernel/debug/clk/clk_summary | grep i2c
                                 enable  prepare  protect                                duty  hardware                            connection
   clock                          count    count    count        rate   accuracy phase  cycle    enable   consumer                         id
    lpi2c8                           0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
    lpi2c7                           0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
    lpi2c6                           0       2        1        24000000    0          0     50000      Y      i2c@426c0000                    no_connection_id
    lpi2c5                           0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
    lpi2c4                           0       2        1        24000000    0          0     50000      Y      i2c@42540000                    no_connection_id
    lpi2c3                           0       2        1        24000000    0          0     50000      Y      i2c@42530000                    no_connection_id
    lpi2c2                           0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
    lpi2c1                           0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
root@imx943evk:~# cat /sys/devices/platform/soc/42000000.bus/42540000.i2c/power/runtime_status
suspended

Add debug log, when defer probe happened on imx-lpi2c 42540000.i2c.
goto rpm_disable path: 

rpm_disable:
	pm_runtime_put(&pdev->dev);
	pm_runtime_disable(&pdev->dev);
	pm_runtime_dont_use_autosuspend(&pdev->dev);

pm_runtime_put() may not work really sometimes.

[    3.203715] imx-lpi2c 42530000.i2c: defer probe
[    3.208324] imx-lpi2c 42530000.i2c: lpi2c_runtime_suspend
[    3.214801] imx-lpi2c 42540000.i2c: defer probe
---> (No lpi2c_runtime_suspend callback(pm_runtime_put() is not really wrok))
[    3.220672] imx-lpi2c 426c0000.i2c: defer probe
[    3.225248] imx-lpi2c 426c0000.i2c: lpi2c_runtime_suspend

After apply this change:
rpm_disable:
	pm_runtime_dont_use_autosuspend(&pdev->dev);
	pm_runtime_put_sync(&pdev->dev);
	pm_runtime_disable(&pdev->dev);

all issues gone.
[    3.093025] imx-lpi2c 42530000.i2c: defer probe
[    3.097592] imx-lpi2c 42530000.i2c: lpi2c_runtime_suspend
[    3.104281] imx-lpi2c 42540000.i2c: defer probe
[    3.108858] imx-lpi2c 42540000.i2c: lpi2c_runtime_suspend
[    3.115278] imx-lpi2c 426c0000.i2c: defer probe
[    3.119818] imx-lpi2c 426c0000.i2c: lpi2c_runtime_suspend

root@imx943evk:~# cat /sys/kernel/debug/clk/clk_summary | grep i2c
                                 enable  prepare  protect                                duty  hardware                            connection
   clock                          count    count    count        rate   accuracy phase  cycle    enable   consumer                         id
    lpi2c8                           0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
    lpi2c7                           0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
    lpi2c6                           0       2        1        24000000    0          0     50000      Y      i2c@426c0000                    no_connection_id
    lpi2c5                           0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
    lpi2c4                           0       2        1        24000000    0          0     50000      Y      i2c@42540000                    no_connection_id
    lpi2c3                           0       2        1        24000000    0          0     50000      Y      i2c@42530000                    no_connection_id
    lpi2c2                           0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
    lpi2c1                           0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id


> > Signed-off-by: Carlos Song <carlos.song@nxp.com>
> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> > Signed-off-by: Jun Li <jun.li@nxp.com>
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> 
> Carlo's SoB should be at the end of the chain. Should be nice to know what these
> are, though, are they co-developed-by? tested-by?
> Why so many SoB's?
> 

This patch author is not me and they meet similar issue at some boards(Vague history), now
I meet this issue at new SOC again. I think this local patch is helpful and looks reasonable.
So I send the patch to community adding my SoB.

> (no need to resend for this)
> 
> Andi
Andi Shyti April 28, 2025, 7:17 p.m. UTC | #3
Hi Carlos,

> rpm_disable:
> 	pm_runtime_put(&pdev->dev);
> 	pm_runtime_disable(&pdev->dev);
> 	pm_runtime_dont_use_autosuspend(&pdev->dev);
> 
> pm_runtime_put() may not work really sometimes.
> 
> [    3.203715] imx-lpi2c 42530000.i2c: defer probe
> [    3.208324] imx-lpi2c 42530000.i2c: lpi2c_runtime_suspend
> [    3.214801] imx-lpi2c 42540000.i2c: defer probe
> ---> (No lpi2c_runtime_suspend callback(pm_runtime_put() is not really wrok))
> [    3.220672] imx-lpi2c 426c0000.i2c: defer probe
> [    3.225248] imx-lpi2c 426c0000.i2c: lpi2c_runtime_suspend
> 
> After apply this change:
> rpm_disable:
> 	pm_runtime_dont_use_autosuspend(&pdev->dev);
> 	pm_runtime_put_sync(&pdev->dev);
> 	pm_runtime_disable(&pdev->dev);
> 
> all issues gone.
> [    3.093025] imx-lpi2c 42530000.i2c: defer probe
> [    3.097592] imx-lpi2c 42530000.i2c: lpi2c_runtime_suspend
> [    3.104281] imx-lpi2c 42540000.i2c: defer probe
> [    3.108858] imx-lpi2c 42540000.i2c: lpi2c_runtime_suspend
> [    3.115278] imx-lpi2c 426c0000.i2c: defer probe
> [    3.119818] imx-lpi2c 426c0000.i2c: lpi2c_runtime_suspend

Thanks for the explanation. If we lose the count here, I think we
also need for the Fixes tag, do you agree? Should we add:

Fixes: 13d6eb20fc79 ("i2c: imx-lpi2c: add runtime pm support")
Cc: <stable@vger.kernel.org> # v4.16+

as well?

...

> > > Signed-off-by: Carlos Song <carlos.song@nxp.com>
> > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> > > Signed-off-by: Jun Li <jun.li@nxp.com>
> > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > 
> > Carlo's SoB should be at the end of the chain. Should be nice to know what these
> > are, though, are they co-developed-by? tested-by?
> > Why so many SoB's?
> > 
> 
> This patch author is not me and they meet similar issue at some boards(Vague history), now
> I meet this issue at new SOC again. I think this local patch is helpful and looks reasonable.
> So I send the patch to community adding my SoB.

So Clark has authored the patch and you have sent it. And your
SoB makes sense (even though, your SoB shoud be placed at the
end, because you sent the patch).

Juan and Haibo have tested it? Reported it?

A practical rule of thumb for tags is that they are placed in
chronological order, e.g.:

 Reported-by: <reporter of the bug>
 Fixes: <commit that introduced the bug>
 Closes: <link where the bug has been reported>
 Co-developed-by: <co-developer>
 Signed-off-by: <co-developer>
 Signed-off-by: <developer>
 Cc: <people who should be aware of the patch>
 Tested-by: <tester>
 Reviewed-by: <reviewer>
 Acked-by: <someone who agrees>
 Signed-off-by: <patch sender>
 Link: <lore link>
 Signed-off-by: <maintainer who applies the patch>

If you notice, more or less things are in chronological order. I
hope this makes it a bit clearer.

Andi
Carlos Song April 29, 2025, 2:15 a.m. UTC | #4
> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Tuesday, April 29, 2025 3:17 AM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: Aisheng Dong <aisheng.dong@nxp.com>; Frank Li <frank.li@nxp.com>;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; linux-i2c@vger.kernel.org; imx@lists.linux.dev;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH] i2c: imx-lpi2c: Fix clock count when probe defers
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
> 
> 
> Hi Carlos,
> 
> > rpm_disable:
> >       pm_runtime_put(&pdev->dev);
> >       pm_runtime_disable(&pdev->dev);
> >       pm_runtime_dont_use_autosuspend(&pdev->dev);
> >
> > pm_runtime_put() may not work really sometimes.
> >
> > [    3.203715] imx-lpi2c 42530000.i2c: defer probe
> > [    3.208324] imx-lpi2c 42530000.i2c: lpi2c_runtime_suspend
> > [    3.214801] imx-lpi2c 42540000.i2c: defer probe
> > ---> (No lpi2c_runtime_suspend callback(pm_runtime_put() is not really
> > ---> wrok))
> > [    3.220672] imx-lpi2c 426c0000.i2c: defer probe
> > [    3.225248] imx-lpi2c 426c0000.i2c: lpi2c_runtime_suspend
> >
> > After apply this change:
> > rpm_disable:
> >       pm_runtime_dont_use_autosuspend(&pdev->dev);
> >       pm_runtime_put_sync(&pdev->dev);
> >       pm_runtime_disable(&pdev->dev);
> >
> > all issues gone.
> > [    3.093025] imx-lpi2c 42530000.i2c: defer probe
> > [    3.097592] imx-lpi2c 42530000.i2c: lpi2c_runtime_suspend
> > [    3.104281] imx-lpi2c 42540000.i2c: defer probe
> > [    3.108858] imx-lpi2c 42540000.i2c: lpi2c_runtime_suspend
> > [    3.115278] imx-lpi2c 426c0000.i2c: defer probe
> > [    3.119818] imx-lpi2c 426c0000.i2c: lpi2c_runtime_suspend
> 
> Thanks for the explanation. If we lose the count here, I think we also need for
> the Fixes tag, do you agree? Should we add:
> 
> Fixes: 13d6eb20fc79 ("i2c: imx-lpi2c: add runtime pm support")
> Cc: <stable@vger.kernel.org> # v4.16+
> 
> as well?
> 
> ...

Hi, Andi

Yes. You are right We should add it.

> 
> > > > Signed-off-by: Carlos Song <carlos.song@nxp.com>
> > > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> > > > Signed-off-by: Jun Li <jun.li@nxp.com>
> > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > >
> > > Carlo's SoB should be at the end of the chain. Should be nice to
> > > know what these are, though, are they co-developed-by? tested-by?
> > > Why so many SoB's?
> > >
> >
> > This patch author is not me and they meet similar issue at some
> > boards(Vague history), now I meet this issue at new SOC again. I think this local
> patch is helpful and looks reasonable.
> > So I send the patch to community adding my SoB.
> 
> So Clark has authored the patch and you have sent it. And your SoB makes sense
> (even though, your SoB shoud be placed at the end, because you sent the
> patch).
> 
> Juan and Haibo have tested it? Reported it?
> 

They are local reviewer. So I change their Reviewed-by: to Signed-off-by:.
Simply you can remove Jun and Haibo tags only keep two tags:

Signed-off-by: Carlos Song <carlos.song@nxp.com>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>

Thank you!

> A practical rule of thumb for tags is that they are placed in chronological order,
> e.g.:
> 
>  Reported-by: <reporter of the bug>
>  Fixes: <commit that introduced the bug>
>  Closes: <link where the bug has been reported>
>  Co-developed-by: <co-developer>
>  Signed-off-by: <co-developer>
>  Signed-off-by: <developer>
>  Cc: <people who should be aware of the patch>
>  Tested-by: <tester>
>  Reviewed-by: <reviewer>
>  Acked-by: <someone who agrees>
>  Signed-off-by: <patch sender>
>  Link: <lore link>
>  Signed-off-by: <maintainer who applies the patch>
> 
> If you notice, more or less things are in chronological order. I hope this makes it
> a bit clearer.
> 
> Andi
Andi Shyti April 29, 2025, 8:01 a.m. UTC | #5
Hi Carlos,

...

> > > > > Signed-off-by: Carlos Song <carlos.song@nxp.com>
> > > > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> > > > > Signed-off-by: Jun Li <jun.li@nxp.com>
> > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > > >
> > > > Carlo's SoB should be at the end of the chain. Should be nice to
> > > > know what these are, though, are they co-developed-by? tested-by?
> > > > Why so many SoB's?
> > > >
> > >
> > > This patch author is not me and they meet similar issue at some
> > > boards(Vague history), now I meet this issue at new SOC again. I think this local
> > patch is helpful and looks reasonable.
> > > So I send the patch to community adding my SoB.
> > 
> > So Clark has authored the patch and you have sent it. And your SoB makes sense
> > (even though, your SoB shoud be placed at the end, because you sent the
> > patch).
> > 
> > Juan and Haibo have tested it? Reported it?
> > 
> 
> They are local reviewer. So I change their Reviewed-by: to Signed-off-by:.
> Simply you can remove Jun and Haibo tags only keep two tags:

I see. Jun and Haibo are welcome to review in this list, if they
want.

> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>

Following the chronological order concept, it would be:

Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Signed-off-by: Carlos Song <carlos.song@nxp.com>

:-)

I'll take care of it.

Thanks,
Andi
Carlos Song April 29, 2025, 8:25 a.m. UTC | #6
> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Tuesday, April 29, 2025 4:21 PM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: Aisheng Dong <aisheng.dong@nxp.com>; Frank Li <frank.li@nxp.com>;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; linux-i2c@vger.kernel.org; imx@lists.linux.dev;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH] i2c: imx-lpi2c: Fix clock count when probe defers
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
> 
> 
> Hi Carlos,
> 
> On Mon, Apr 21, 2025 at 02:23:41PM +0800, carlos.song@nxp.com wrote:
> > From: Clark Wang <xiaoning.wang@nxp.com>
> >
> > Deferred probe with pm_runtime_put() may delay clock disable, causing
> > incorrect clock usage count. Use pm_runtime_put_sync() to ensure the
> > clock is disabled immediately.
> >
> > Signed-off-by: Carlos Song <carlos.song@nxp.com>
> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> > Signed-off-by: Jun Li <jun.li@nxp.com>
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> 
> merged to i2c/i2c-host-fixes with all the changes I suggested.
> 

That's good! Thank you very much!

> Thanks,
> Andi
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 0d4b3935e687..342d47e67586 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -1380,9 +1380,9 @@  static int lpi2c_imx_probe(struct platform_device *pdev)
 	return 0;
 
 rpm_disable:
-	pm_runtime_put(&pdev->dev);
-	pm_runtime_disable(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 
 	return ret;
 }