From: Alexander Stein <alexander.stein@ew.tq-group.com>
To: Dong Aisheng <aisheng.dong@nxp.com>,
Andi Shyti <andi.shyti@kernel.org>,
Shawn Guo <shawnguo@kernel.org>,
Sascha Hauer <s.hauer@pengutronix.de>,
Fabio Estevam <festevam@gmail.com>,
Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: "Alexander Stein" <alexander.stein@ew.tq-group.com>,
"NXP Linux Team" <linux-imx@nxp.com>,
linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
imx@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: [PATCH v3 1/1] i2c: lpi2c: Avoid calling clk_get_rate during transfer
Date: Mon, 22 Apr 2024 13:36:29 +0200 [thread overview]
Message-ID: <20240422113629.1629891-1-alexander.stein@ew.tq-group.com> (raw)
Instead of repeatedly calling clk_get_rate for each transfer, lock
the clock rate and cache the value.
A deadlock has been observed while adding tlv320aic32x4 audio codec to
the system. When this clock provider adds its clock, the clk mutex is
locked already, it needs to access i2c, which in return needs the mutex
for clk_get_rate as well.
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
---
This is an alternative, lightweight approach replacing the patch [1].
The issue to address is still removing the call to clk_get_rate() during each
transfer, which might reuslt in a deadlock. lockdep also complains about this
call chain.
The dependency from v2 has already been merged in commit b0cde62e4c548
("clk: Add a devm variant of clk_rate_exclusive_get()").
Instead of adding a clock notifier, lock the peripheral clock rate and cache
the peripheral clock rate.
Currently LPI2C is available in the following SoC:
* i.MX7ULP
* i.MX8ULP
* i.MX8DXL
* i.MX8X
* i.MX8
* i.MX93
Additionally I expect both i.MX91 and i.MX95 to also use this driver.
This patch assumes the parent clock rate never changes. This is apparently true
for i.MX93 as each I2C has it's own lpi2c*_root clock. On i.MX8 and i.MX8X
clocks are managed by SCU with it's own dedicated firmware. I can't say if the
clock never changes though. I have no idea about the other SoC.
Changes in v3:
* Rebased to next-20240422
Changes in v2:
* Removed redundent clk_rate check in lpi2c_imx_config (I opted to keep
the local variable to not extent the calculation code lines)
* Collected R-b
Best regards,
Alexander
[1] https://lore.kernel.org/all/20240110120556.519800-1-alexander.stein@ew.tq-group.com/
drivers/i2c/busses/i2c-imx-lpi2c.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 6d72e4e126dde..36e8f6196a87b 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -99,6 +99,7 @@ struct lpi2c_imx_struct {
__u8 *rx_buf;
__u8 *tx_buf;
struct completion complete;
+ unsigned long rate_per;
unsigned int msglen;
unsigned int delivered;
unsigned int block_data;
@@ -212,9 +213,7 @@ static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
lpi2c_imx_set_mode(lpi2c_imx);
- clk_rate = clk_get_rate(lpi2c_imx->clks[0].clk);
- if (!clk_rate)
- return -EINVAL;
+ clk_rate = lpi2c_imx->rate_per;
if (lpi2c_imx->mode == HS || lpi2c_imx->mode == ULTRA_FAST)
filt = 0;
@@ -611,6 +610,20 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
if (ret)
return ret;
+ /*
+ * Lock the parent clock rate to avoid getting parent clock upon
+ * each transfer
+ */
+ ret = devm_clk_rate_exclusive_get(&pdev->dev, lpi2c_imx->clks[0].clk);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "can't lock I2C peripheral clock rate\n");
+
+ lpi2c_imx->rate_per = clk_get_rate(lpi2c_imx->clks[0].clk);
+ if (!lpi2c_imx->rate_per)
+ return dev_err_probe(&pdev->dev, -EINVAL,
+ "can't get I2C peripheral clock rate\n");
+
pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_get_noresume(&pdev->dev);
--
2.34.1
next reply other threads:[~2024-04-22 11:36 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-22 11:36 Alexander Stein [this message]
2024-04-22 15:42 ` [PATCH v3 1/1] i2c: lpi2c: Avoid calling clk_get_rate during transfer Uwe Kleine-König
2024-04-22 22:25 ` Andi Shyti
2024-04-22 22:51 ` (subset) " Andi Shyti
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240422113629.1629891-1-alexander.stein@ew.tq-group.com \
--to=alexander.stein@ew.tq-group.com \
--cc=aisheng.dong@nxp.com \
--cc=andi.shyti@kernel.org \
--cc=festevam@gmail.com \
--cc=imx@lists.linux.dev \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=s.hauer@pengutronix.de \
--cc=shawnguo@kernel.org \
--cc=u.kleine-koenig@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).