[v2,1/1] i2c: lpi2c: Avoid calling clk_get_rate during transfer

Commit Message

Alexander Stein Jan. 19, 2024, 7:22 a.m. UTC

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 <[e-mail]>
Reviewed-by: Uwe Kleine-König <[e-mail]>
Reviewed-by: Andi Shyti <[e-mail]>
---
This is an alternative, lightweight approach replacing the patch [1] and
depends on [2].
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.

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 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/[e-mail]/
[2] https://lore.kernel.org/all/[e-mail]/

 drivers/i2c/busses/i2c-imx-lpi2c.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Comments

Uwe Kleine-König Jan. 19, 2024, 7:48 a.m. UTC | #1

Hello Alexander,

On Fri, Jan 19, 2024 at 08:22:23AM +0100, Alexander Stein wrote:
> 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 <[e-mail]>
> Reviewed-by: Uwe Kleine-König <[e-mail]>
> Reviewed-by: Andi Shyti <[e-mail]>
> ---
> This is an alternative, lightweight approach replacing the patch [1] and
> depends on [2].

If the kernel build bots find your patch they will likely trip over not
being able to parse that. If you want to prevent this, you could make
use of git format-patch's --base parameter.

Best regards
Uwe

Patch

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 678b30e90492a..14473c223cfa7 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;
@@ -207,9 +208,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;
@@ -590,6 +589,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);