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

[Uwe Kleine-König]

On Thu, Jan 18, 2024 at 08:43:32AM +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 <alexander.stein at ew.tq-group.com>
> ---
> 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.
> 
> Best regards,
> Alexander
> 
> [1] https://lore.kernel.org/all/20240110120556.519800-1-alexander.stein@ew.tq-group.com/
> [2] https://lore.kernel.org/all/20240104225512.1124519-2-u.kleine-koenig@pengutronix.de/
> 
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 678b30e90492a..6cbcb27a3b280 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,7 +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);
> +	clk_rate = lpi2c_imx->rate_per;
>  	if (!clk_rate)
>  		return -EINVAL;

After the things you did in lpi2c_imx_probe() you can assume that
clk_rate is not zero, so you could drop the if here.

Otherwise looks good to me (if you want even if you keep the if which is
only a minor optimisation).

Reviewed-by: Uwe Kleine-König <u.kleine-koenig at pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20240118/db3b813c/attachment.sig>