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@xxxxxxxxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxx/ > [2] https://lore.kernel.org/all/20240104225512.1124519-2-u.kleine-koenig@xxxxxxxxxxxxxx/ > > 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@xxxxxxxxxxxxxx> Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
