[PATCH 1/1] i2c: lpi2c: Avoid calling clk_get_rate during transfer
Uwe Kleine-König
u.kleine-koenig at pengutronix.de
Thu Jan 18 03:53:46 PST 2024
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 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@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/ |
-------------- 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/b957910a/attachment.sig>
More information about the linux-arm-kernel
mailing list