| Message ID | 20250421062341.2471922-1-carlos.song@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: imx-lpi2c: Fix clock count when probe defers | expand |
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
> -----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
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
> -----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
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
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. Thanks, Andi
> -----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 --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; }