| Message ID | 20240118074332.3633533-1-alexander.stein@ew.tq-group.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] i2c: lpi2c: Avoid calling clk_get_rate during transfer | expand |
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@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@pengutronix.de> Best regards Uwe
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@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. > 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@pengutronix.de> Thanks and best regards, Alexander > Best regards > Uwe
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@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
Hi, > > 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@pengutronix.de> you're finding users already... :-) Looks better than the original version. Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
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; @@ -590,6 +591,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);
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@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(-)