On Thu, Jan 18, 2024 at 12:26:39PM +0100, Alexander Stein wrote: > Hi Uwe, > > Am Donnerstag, 18. Januar 2024, 11:22:35 CET schrieb 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@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@xxxxx > > > -group.com/ [2] > > > https://lore.kernel.org/all/20240104225512.1124519-2-u.kleine-koenig@peng > > > utronix.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. > > As in some cases the clock is not setup by Linux, but externally, I'd rather > keep that check to ensure it's enabled. Just to be sure we both understand this in the same way: In .probe() you do: lpi2c_imx->rate_per = clk_get_rate(lpi2c_imx->clks[0].clk); if (!lpi2c_imx->rate_per) return dev_err_probe(&pdev->dev, -EINVAL, ...); so irrespective of how the clock is setup, I would expect that lpi2c_imx->rate_per will never be zero in lpi2c_imx_config(). Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
