[PATCH v7 1/1] i2c: lpi2c: use clk notifier for rate changes

[PATCH v7 1/1] i2c: lpi2c: use clk notifier for rate changes

[Alexander Stein]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Instead of repeatedly calling clk_get_rate for each transfer, register
a clock notifier to update the cached divider value each time the clock
rate actually changes.
There is also a lockdep splat if a I2C based clock provider needs to
access the i2c bus while in recalc_rate(). "prepare_lock" is about to
be locked recursively.

Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in v7:
* Use dev_err_probe
* Reworked/Shortened the commit message
 It is not about saving CPU cycles, but to avoid locking the clk subsystem
 upon each transfer.

 drivers/i2c/busses/i2c-imx-lpi2c.c | 40 +++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 678b30e90492a..3070e605fd8ff 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -5,6 +5,7 @@
  * Copyright 2016 Freescale Semiconductor, Inc.
  */
 
+#include <linux/atomic.h>
 #include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
@@ -99,6 +100,8 @@ struct lpi2c_imx_struct {
 	__u8			*rx_buf;
 	__u8			*tx_buf;
 	struct completion	complete;
+	struct notifier_block	clk_change_nb;
+	atomic_t		rate_per;
 	unsigned int		msglen;
 	unsigned int		delivered;
 	unsigned int		block_data;
@@ -197,6 +200,20 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct *lpi2c_imx)
 	} while (1);
 }
 
+static int lpi2c_imx_clk_change_cb(struct notifier_block *nb,
+				   unsigned long action, void *data)
+{
+	struct clk_notifier_data *ndata = data;
+	struct lpi2c_imx_struct *lpi2c_imx = container_of(nb,
+							  struct lpi2c_imx_struct,
+							  clk_change_nb);
+
+	if (action & POST_RATE_CHANGE)
+		atomic_set(&lpi2c_imx->rate_per, ndata->new_rate);
+
+	return NOTIFY_OK;
+}
+
 /* CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2 */
 static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
 {
@@ -207,7 +224,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 = atomic_read(&lpi2c_imx->rate_per);
 	if (!clk_rate)
 		return -EINVAL;
 
@@ -590,6 +607,27 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	lpi2c_imx->clk_change_nb.notifier_call = lpi2c_imx_clk_change_cb;
+	ret = devm_clk_notifier_register(&pdev->dev, lpi2c_imx->clks[0].clk,
+					 &lpi2c_imx->clk_change_nb);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "can't register peripheral clock notifier\n");
+	/*
+	 * Lock the clock rate to avoid rate change between clk_get_rate() and
+	 * atomic_set()
+	 */
+	ret = clk_rate_exclusive_get(lpi2c_imx->clks[0].clk);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "can't lock I2C peripheral clock rate\n");
+
+	atomic_set(&lpi2c_imx->rate_per, clk_get_rate(lpi2c_imx->clks[0].clk));
+	clk_rate_exclusive_put(lpi2c_imx->clks[0].clk);
+	if (!atomic_read(&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);
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 1/1] i2c: lpi2c: use clk notifier for rate changes

[Andi Shyti]

Hi Alexander,

is it my mail client not working or is is your patch that has
gone through something terribly bad?

Andi

On Tue, Nov 07, 2023 at 03:12:01PM +0100, Alexander Stein wrote:
> * CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2 */
>  static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
>  {
> @@ -207,7 +224,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 = atomic_read(&lpi2c_imx->rate_per);
>  	if (!clk_rate)
>  		return -EINVAL;
>  
> @@ -590,6 +607,27 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	lpi2c_imx->clk_change_nb.notifier_call = lpi2c_imx_clk_change_cb;
> +	ret = devm_clk_notifier_register(&pdev->dev, lpi2c_imx->clks[0].clk,
> +					 &lpi2c_imx->clk_change_nb);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "can't register peripheral clock notifier\n");
> +	/*
> +	 * Lock the clock rate to avoid rate change between clk_get_rate() and
> +	 * atomic_set()
> +	 */
> +	ret = clk_rate_exclusive_get(lpi2c_imx->clks[0].clk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "can't lock I2C peripheral clock rate\n");
> +
> +	atomic_set(&lpi2c_imx->rate_per, clk_get_rate(lpi2c_imx->clks[0].clk));
> +	clk_rate_exclusive_put(lpi2c_imx->clks[0].clk);
> +	if (!atomic_read(&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);
> -- 
> 2.34.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 1/1] i2c: lpi2c: use clk notifier for rate changes

[Alexander Stein]

Hi Andi,

Am Dienstag, 7. November 2023, 22:20:49 CET schrieb Andi Shyti:
> Hi Alexander,
> 
> is it my mail client not working or is is your patch that has
> gone through something terribly bad?

I can't see anything obviously wrong. Can you elaborate?

Thanks
Alexander

> Andi
> 
> On Tue, Nov 07, 2023 at 03:12:01PM +0100, Alexander Stein wrote:
> > * CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2 */
> > 
> >  static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
> >  {
> > 
> > @@ -207,7 +224,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 = atomic_read(&lpi2c_imx->rate_per);
> > 
> >  	if (!clk_rate)
> >  	
> >  		return -EINVAL;
> > 
> > @@ -590,6 +607,27 @@ static int lpi2c_imx_probe(struct platform_device
> > *pdev)> 
> >  	if (ret)
> >  	
> >  		return ret;
> > 
> > +	lpi2c_imx->clk_change_nb.notifier_call = lpi2c_imx_clk_change_cb;
> > +	ret = devm_clk_notifier_register(&pdev->dev, lpi2c_imx->clks[0].clk,
> > +					 &lpi2c_imx->clk_change_nb);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "can't register peripheral clock 
notifier\n");
> > +	/*
> > +	 * Lock the clock rate to avoid rate change between clk_get_rate() 
and
> > +	 * atomic_set()
> > +	 */
> > +	ret = clk_rate_exclusive_get(lpi2c_imx->clks[0].clk);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "can't lock I2C peripheral clock 
rate\n");
> > +
> > +	atomic_set(&lpi2c_imx->rate_per, clk_get_rate(lpi2c_imx-
>clks[0].clk));
> > +	clk_rate_exclusive_put(lpi2c_imx->clks[0].clk);
> > +	if (!atomic_read(&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);


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 1/1] i2c: lpi2c: use clk notifier for rate changes

[Andi Shyti]

Hi Alexander,

On Wed, Nov 08, 2023 at 07:46:12AM +0100, Alexander Stein wrote:
> Am Dienstag, 7. November 2023, 22:20:49 CET schrieb Andi Shyti:
> > is it my mail client not working or is is your patch that has
> > gone through something terribly bad?
> 
> I can't see anything obviously wrong. Can you elaborate?

If you see your part in my reply down here you see that some
parts are missing. Perhaps there is a bug in lei that has missed
the top part of your patch because in lore I see that, indeed,
there is nothing wrong with your mail.

Andi

> Thanks
> Alexander
> 
> > Andi
> > 
> > On Tue, Nov 07, 2023 at 03:12:01PM +0100, Alexander Stein wrote:
> > > * CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2 */
> > > 
> > >  static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
> > >  {
> > > 
> > > @@ -207,7 +224,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 = atomic_read(&lpi2c_imx->rate_per);
> > > 
> > >  	if (!clk_rate)
> > >  	
> > >  		return -EINVAL;
> > > 
> > > @@ -590,6 +607,27 @@ static int lpi2c_imx_probe(struct platform_device
> > > *pdev)> 
> > >  	if (ret)
> > >  	
> > >  		return ret;
> > > 
> > > +	lpi2c_imx->clk_change_nb.notifier_call = lpi2c_imx_clk_change_cb;
> > > +	ret = devm_clk_notifier_register(&pdev->dev, lpi2c_imx->clks[0].clk,
> > > +					 &lpi2c_imx->clk_change_nb);
> > > +	if (ret)
> > > +		return dev_err_probe(&pdev->dev, ret,
> > > +				     "can't register peripheral clock 
> notifier\n");
> > > +	/*
> > > +	 * Lock the clock rate to avoid rate change between clk_get_rate() 
> and
> > > +	 * atomic_set()
> > > +	 */
> > > +	ret = clk_rate_exclusive_get(lpi2c_imx->clks[0].clk);
> > > +	if (ret)
> > > +		return dev_err_probe(&pdev->dev, ret,
> > > +				     "can't lock I2C peripheral clock 
> rate\n");
> > > +
> > > +	atomic_set(&lpi2c_imx->rate_per, clk_get_rate(lpi2c_imx-
> >clks[0].clk));
> > > +	clk_rate_exclusive_put(lpi2c_imx->clks[0].clk);
> > > +	if (!atomic_read(&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);
> 
> 
> -- 
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 1/1] i2c: lpi2c: use clk notifier for rate changes

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 330 bytes --]


> If you see your part in my reply down here you see that some
> parts are missing. Perhaps there is a bug in lei that has missed
> the top part of your patch because in lore I see that, indeed,
> there is nothing wrong with your mail.

Seems to be a lei bug, indeed. I can apply the patch from my mutt inbox
without a problem.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 1/1] i2c: lpi2c: use clk notifier for rate changes

[Andi Shyti]

Hi Wolfram,

> > If you see your part in my reply down here you see that some
> > parts are missing. Perhaps there is a bug in lei that has missed
> > the top part of your patch because in lore I see that, indeed,
> > there is nothing wrong with your mail.
> 
> Seems to be a lei bug, indeed. I can apply the patch from my mutt inbox
> without a problem.

thanks for confirming, I will download it manually from lore and
review it, then.

Thanks,
Andi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 1/1] i2c: lpi2c: use clk notifier for rate changes

[Andi Shyti]

Hi Alexander,

On Tue, Nov 07, 2023 at 03:12:01PM +0100, Alexander Stein wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> Instead of repeatedly calling clk_get_rate for each transfer, register
> a clock notifier to update the cached divider value each time the clock
> rate actually changes.
> There is also a lockdep splat if a I2C based clock provider needs to
> access the i2c bus while in recalc_rate(). "prepare_lock" is about to
> be locked recursively.
> 
> Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")

What's the exact fix here? Is it the lockdep warning? Is it
actually causing a real deadlock?

> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes in v7:
> * Use dev_err_probe
> * Reworked/Shortened the commit message
>  It is not about saving CPU cycles, but to avoid locking the clk subsystem
>  upon each transfer.
> 
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 40 +++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 678b30e90492a..3070e605fd8ff 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -5,6 +5,7 @@
>   * Copyright 2016 Freescale Semiconductor, Inc.
>   */
>  
> +#include <linux/atomic.h>
>  #include <linux/clk.h>
>  #include <linux/completion.h>
>  #include <linux/delay.h>
> @@ -99,6 +100,8 @@ struct lpi2c_imx_struct {
>  	__u8			*rx_buf;
>  	__u8			*tx_buf;
>  	struct completion	complete;
> +	struct notifier_block	clk_change_nb;
> +	atomic_t		rate_per;
>  	unsigned int		msglen;
>  	unsigned int		delivered;
>  	unsigned int		block_data;
> @@ -197,6 +200,20 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct *lpi2c_imx)
>  	} while (1);
>  }
>  
> +static int lpi2c_imx_clk_change_cb(struct notifier_block *nb,
> +				   unsigned long action, void *data)
> +{
> +	struct clk_notifier_data *ndata = data;
> +	struct lpi2c_imx_struct *lpi2c_imx = container_of(nb,
> +							  struct lpi2c_imx_struct,
> +							  clk_change_nb);
> +
> +	if (action & POST_RATE_CHANGE)
> +		atomic_set(&lpi2c_imx->rate_per, ndata->new_rate);
> +
> +	return NOTIFY_OK;
> +}
> +
>  /* CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2 */
>  static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
>  {
> @@ -207,7 +224,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 = atomic_read(&lpi2c_imx->rate_per);
>  	if (!clk_rate)
>  		return -EINVAL;

Doesn't seem like EINVAL, defined as "Invalid argument", is the
correct number here. As we are failing to read the clock rate, do
you think EIO is better?

>  
> @@ -590,6 +607,27 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	lpi2c_imx->clk_change_nb.notifier_call = lpi2c_imx_clk_change_cb;
> +	ret = devm_clk_notifier_register(&pdev->dev, lpi2c_imx->clks[0].clk,
> +					 &lpi2c_imx->clk_change_nb);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "can't register peripheral clock notifier\n");

can't we fall back to how things were instead of failing the
probe? But I'm not sure it would remove the lockdep warning,
though. I can live with it.

> +	/*
> +	 * Lock the clock rate to avoid rate change between clk_get_rate() and
> +	 * atomic_set()
> +	 */
> +	ret = clk_rate_exclusive_get(lpi2c_imx->clks[0].clk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "can't lock I2C peripheral clock rate\n");
> +
> +	atomic_set(&lpi2c_imx->rate_per, clk_get_rate(lpi2c_imx->clks[0].clk));
> +	clk_rate_exclusive_put(lpi2c_imx->clks[0].clk);
> +	if (!atomic_read(&lpi2c_imx->rate_per))
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +				     "can't get I2C peripheral clock rate\n");
> +

Not a bad patch, would be nice if all the above was provided by
the library so that other drivers can use it.

Andi

>  	pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_get_noresume(&pdev->dev);
> -- 
> 2.34.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 1/1] i2c: lpi2c: use clk notifier for rate changes

[Alexander Stein]

Hi Andi,

thanks for the feedback.

Am Donnerstag, 9. November 2023, 00:38:09 CET schrieb Andi Shyti:
> Hi Alexander,
> 
> On Tue, Nov 07, 2023 at 03:12:01PM +0100, Alexander Stein wrote:
> > From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > 
> > Instead of repeatedly calling clk_get_rate for each transfer, register
> > a clock notifier to update the cached divider value each time the clock
> > rate actually changes.
> > There is also a lockdep splat if a I2C based clock provider needs to
> > access the i2c bus while in recalc_rate(). "prepare_lock" is about to
> > be locked recursively.
> > 
> > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> 
> What's the exact fix here? Is it the lockdep warning? Is it
> actually causing a real deadlock?

We've seen actual deadlocks on our imx8qxp based hardware using a downstream 
kernel, so we have implemented as similar fix [1]. This happened using 
tlv320aic32x4 audio codec.
The backtrace is similar, a i2c based clock provider is trying t issue an i2c 
transfer while adding the clock, thus 'prepare_lock' is already locked.
Lockdep raises an error both for downstream kernel as well as mainline, both 
for tlv320aic32x4 or pcf85063.

[1] https://github.com/tq-systems/linux-tqmaxx/commit/
b0339ff83a979f2ea066012b9209ea7c54f2b4e7

> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Changes in v7:
> > * Use dev_err_probe
> > * Reworked/Shortened the commit message
> > 
> >  It is not about saving CPU cycles, but to avoid locking the clk subsystem
> >  upon each transfer.
> >  
> >  drivers/i2c/busses/i2c-imx-lpi2c.c | 40 +++++++++++++++++++++++++++++-
> >  1 file changed, 39 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > b/drivers/i2c/busses/i2c-imx-lpi2c.c index 678b30e90492a..3070e605fd8ff
> > 100644
> > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > @@ -5,6 +5,7 @@
> > 
> >   * Copyright 2016 Freescale Semiconductor, Inc.
> >   */
> > 
> > +#include <linux/atomic.h>
> > 
> >  #include <linux/clk.h>
> >  #include <linux/completion.h>
> >  #include <linux/delay.h>
> > 
> > @@ -99,6 +100,8 @@ struct lpi2c_imx_struct {
> > 
> >  	__u8			*rx_buf;
> >  	__u8			*tx_buf;
> >  	struct completion	complete;
> > 
> > +	struct notifier_block	clk_change_nb;
> > +	atomic_t		rate_per;
> > 
> >  	unsigned int		msglen;
> >  	unsigned int		delivered;
> >  	unsigned int		block_data;
> > 
> > @@ -197,6 +200,20 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct
> > *lpi2c_imx)> 
> >  	} while (1);
> >  
> >  }
> > 
> > +static int lpi2c_imx_clk_change_cb(struct notifier_block *nb,
> > +				   unsigned long action, void *data)
> > +{
> > +	struct clk_notifier_data *ndata = data;
> > +	struct lpi2c_imx_struct *lpi2c_imx = container_of(nb,
> > +							  struct 
lpi2c_imx_struct,
> > +							  
clk_change_nb);
> > +
> > +	if (action & POST_RATE_CHANGE)
> > +		atomic_set(&lpi2c_imx->rate_per, ndata->new_rate);
> > +
> > +	return NOTIFY_OK;
> > +}
> > +
> > 
> >  /* CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2 */
> >  static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
> >  {
> > 
> > @@ -207,7 +224,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 = atomic_read(&lpi2c_imx->rate_per);
> > 
> >  	if (!clk_rate)
> >  	
> >  		return -EINVAL;
> 
> Doesn't seem like EINVAL, defined as "Invalid argument", is the
> correct number here. As we are failing to read the clock rate, do
> you think EIO is better?

Well, this is already the current error code. In both the old and new code I 
would consider this error case as 'no clock rate provided' rather than failing 
to read. I would reject EIO as there is no IO transfer for retrieving the 
clock rate.

> > @@ -590,6 +607,27 @@ static int lpi2c_imx_probe(struct platform_device
> > *pdev)> 
> >  	if (ret)
> >  	
> >  		return ret;
> > 
> > +	lpi2c_imx->clk_change_nb.notifier_call = lpi2c_imx_clk_change_cb;
> > +	ret = devm_clk_notifier_register(&pdev->dev, lpi2c_imx->clks[0].clk,
> > +					 &lpi2c_imx->clk_change_nb);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "can't register peripheral clock 
notifier\n");
> 
> can't we fall back to how things were instead of failing the
> probe? But I'm not sure it would remove the lockdep warning,
> though. I can live with it.

I don't see a reason why you would want to continue if 
devm_clk_notifier_register() failed. It's either ENOMEM, EINVAL (if you pass 
NULL for clk or notifier block) or EEXIST if registered twice. So in reality 
only ENOMEM is possible, but then things went south already.

Best regards,
Alexander

> > +	/*
> > +	 * Lock the clock rate to avoid rate change between clk_get_rate() 
and
> > +	 * atomic_set()
> > +	 */
> > +	ret = clk_rate_exclusive_get(lpi2c_imx->clks[0].clk);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "can't lock I2C peripheral clock 
rate\n");
> > +
> > +	atomic_set(&lpi2c_imx->rate_per, clk_get_rate(lpi2c_imx-
>clks[0].clk));
> > +	clk_rate_exclusive_put(lpi2c_imx->clks[0].clk);
> > +	if (!atomic_read(&lpi2c_imx->rate_per))
> > +		return dev_err_probe(&pdev->dev, -EINVAL,
> > +				     "can't get I2C peripheral clock 
rate\n");
> > +
> 
> Not a bad patch, would be nice if all the above was provided by
> the library so that other drivers can use it.
> 
> Andi
> 
> >  	pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
> >  	pm_runtime_use_autosuspend(&pdev->dev);
> >  	pm_runtime_get_noresume(&pdev->dev);


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 1/1] i2c: lpi2c: use clk notifier for rate changes

[Andi Shyti]

Hi Alexander,

On Thu, Nov 09, 2023 at 08:54:51AM +0100, Alexander Stein wrote:
> Am Donnerstag, 9. November 2023, 00:38:09 CET schrieb Andi Shyti:
> > On Tue, Nov 07, 2023 at 03:12:01PM +0100, Alexander Stein wrote:
> > > Instead of repeatedly calling clk_get_rate for each transfer, register
> > > a clock notifier to update the cached divider value each time the clock
> > > rate actually changes.
> > > There is also a lockdep splat if a I2C based clock provider needs to
> > > access the i2c bus while in recalc_rate(). "prepare_lock" is about to
> > > be locked recursively.
> > > 
> > > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> > 
> > What's the exact fix here? Is it the lockdep warning? Is it
> > actually causing a real deadlock?
> 
> We've seen actual deadlocks on our imx8qxp based hardware using a downstream 
> kernel, so we have implemented as similar fix [1]. This happened using 
> tlv320aic32x4 audio codec.
> The backtrace is similar, a i2c based clock provider is trying t issue an i2c 
> transfer while adding the clock, thus 'prepare_lock' is already locked.
> Lockdep raises an error both for downstream kernel as well as mainline, both 
> for tlv320aic32x4 or pcf85063.

yes, if the clock is using the same controller then it's likely
that you might end up in a deadlock. This is why I like this
patch and I believe this shouild go in the library at some point.

But the deadlock is not mentioned in the commit log and lockdep
doesn't always mean deadlock.

> [1] https://github.com/tq-systems/linux-tqmaxx/commit/
> b0339ff83a979f2ea066012b9209ea7c54f2b4e7
> 
> > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > > Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > ---
> > > Changes in v7:
> > > * Use dev_err_probe
> > > * Reworked/Shortened the commit message
> > > 
> > >  It is not about saving CPU cycles, but to avoid locking the clk subsystem
> > >  upon each transfer.
> > >  
> > >  drivers/i2c/busses/i2c-imx-lpi2c.c | 40 +++++++++++++++++++++++++++++-
> > >  1 file changed, 39 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > > b/drivers/i2c/busses/i2c-imx-lpi2c.c index 678b30e90492a..3070e605fd8ff
> > > 100644
> > > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > > @@ -5,6 +5,7 @@
> > > 
> > >   * Copyright 2016 Freescale Semiconductor, Inc.
> > >   */
> > > 
> > > +#include <linux/atomic.h>
> > > 
> > >  #include <linux/clk.h>
> > >  #include <linux/completion.h>
> > >  #include <linux/delay.h>
> > > 
> > > @@ -99,6 +100,8 @@ struct lpi2c_imx_struct {
> > > 
> > >  	__u8			*rx_buf;
> > >  	__u8			*tx_buf;
> > >  	struct completion	complete;
> > > 
> > > +	struct notifier_block	clk_change_nb;
> > > +	atomic_t		rate_per;
> > > 
> > >  	unsigned int		msglen;
> > >  	unsigned int		delivered;
> > >  	unsigned int		block_data;
> > > 
> > > @@ -197,6 +200,20 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct
> > > *lpi2c_imx)> 
> > >  	} while (1);
> > >  
> > >  }
> > > 
> > > +static int lpi2c_imx_clk_change_cb(struct notifier_block *nb,
> > > +				   unsigned long action, void *data)
> > > +{
> > > +	struct clk_notifier_data *ndata = data;
> > > +	struct lpi2c_imx_struct *lpi2c_imx = container_of(nb,
> > > +							  struct 
> lpi2c_imx_struct,
> > > +							  
> clk_change_nb);
> > > +
> > > +	if (action & POST_RATE_CHANGE)
> > > +		atomic_set(&lpi2c_imx->rate_per, ndata->new_rate);
> > > +
> > > +	return NOTIFY_OK;
> > > +}
> > > +
> > > 
> > >  /* CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2 */
> > >  static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
> > >  {
> > > 
> > > @@ -207,7 +224,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 = atomic_read(&lpi2c_imx->rate_per);
> > > 
> > >  	if (!clk_rate)
> > >  	
> > >  		return -EINVAL;
> > 
> > Doesn't seem like EINVAL, defined as "Invalid argument", is the
> > correct number here. As we are failing to read the clock rate, do
> > you think EIO is better?
> 
> Well, this is already the current error code. In both the old and new code I 
> would consider this error case as 'no clock rate provided' rather than failing 
> to read. I would reject EIO as there is no IO transfer for retrieving the 
> clock rate.

It's definitely not EINVAL as we are failing not because of
invalid arguments. I thought of EIO because at some point the
clock rate has been retrieved (or set, thus i/o) and "rate_per"
updated accordingly. But I agree that's not the perfect value
either.

I couldn't think of a better error value, though.

> > > @@ -590,6 +607,27 @@ static int lpi2c_imx_probe(struct platform_device
> > > *pdev)> 
> > >  	if (ret)
> > >  	
> > >  		return ret;
> > > 
> > > +	lpi2c_imx->clk_change_nb.notifier_call = lpi2c_imx_clk_change_cb;
> > > +	ret = devm_clk_notifier_register(&pdev->dev, lpi2c_imx->clks[0].clk,
> > > +					 &lpi2c_imx->clk_change_nb);
> > > +	if (ret)
> > > +		return dev_err_probe(&pdev->dev, ret,
> > > +				     "can't register peripheral clock 
> notifier\n");
> > 
> > can't we fall back to how things were instead of failing the
> > probe? But I'm not sure it would remove the lockdep warning,
> > though. I can live with it.
> 
> I don't see a reason why you would want to continue if 
> devm_clk_notifier_register() failed. It's either ENOMEM, EINVAL (if you pass 
> NULL for clk or notifier block) or EEXIST if registered twice. So in reality 
> only ENOMEM is possible, but then things went south already.

why do you care? If ENOMEM has failed, then let the driver fail
on its own, don't be the one pulling the trigger.

But I'm not strong here: it's not completely wrong to bail out,
either.

Andi

> Best regards,
> Alexander
> 
> > > +	/*
> > > +	 * Lock the clock rate to avoid rate change between clk_get_rate() 
> and
> > > +	 * atomic_set()
> > > +	 */
> > > +	ret = clk_rate_exclusive_get(lpi2c_imx->clks[0].clk);
> > > +	if (ret)
> > > +		return dev_err_probe(&pdev->dev, ret,
> > > +				     "can't lock I2C peripheral clock 
> rate\n");
> > > +
> > > +	atomic_set(&lpi2c_imx->rate_per, clk_get_rate(lpi2c_imx-
> >clks[0].clk));
> > > +	clk_rate_exclusive_put(lpi2c_imx->clks[0].clk);
> > > +	if (!atomic_read(&lpi2c_imx->rate_per))
> > > +		return dev_err_probe(&pdev->dev, -EINVAL,
> > > +				     "can't get I2C peripheral clock 
> rate\n");
> > > +
> > 
> > Not a bad patch, would be nice if all the above was provided by
> > the library so that other drivers can use it.
> > 
> > Andi
> > 
> > >  	pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
> > >  	pm_runtime_use_autosuspend(&pdev->dev);
> > >  	pm_runtime_get_noresume(&pdev->dev);
> 
> 
> -- 
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 1/1] i2c: lpi2c: use clk notifier for rate changes

[Andi Shyti]

Alexander,

if you...

On Thu, Nov 09, 2023 at 10:10:46AM +0100, Andi Shyti wrote:
> On Thu, Nov 09, 2023 at 08:54:51AM +0100, Alexander Stein wrote:
> > Am Donnerstag, 9. November 2023, 00:38:09 CET schrieb Andi Shyti:
> > > On Tue, Nov 07, 2023 at 03:12:01PM +0100, Alexander Stein wrote:
> > > > Instead of repeatedly calling clk_get_rate for each transfer, register
> > > > a clock notifier to update the cached divider value each time the clock
> > > > rate actually changes.
> > > > There is also a lockdep splat if a I2C based clock provider needs to
> > > > access the i2c bus while in recalc_rate(). "prepare_lock" is about to
> > > > be locked recursively.
> > > > 
> > > > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> > > 
> > > What's the exact fix here? Is it the lockdep warning? Is it
> > > actually causing a real deadlock?
> > 
> > We've seen actual deadlocks on our imx8qxp based hardware using a downstream 
> > kernel, so we have implemented as similar fix [1]. This happened using 
> > tlv320aic32x4 audio codec.
> > The backtrace is similar, a i2c based clock provider is trying t issue an i2c 
> > transfer while adding the clock, thus 'prepare_lock' is already locked.
> > Lockdep raises an error both for downstream kernel as well as mainline, both 
> > for tlv320aic32x4 or pcf85063.
> 
> yes, if the clock is using the same controller then it's likely
> that you might end up in a deadlock. This is why I like this
> patch and I believe this shouild go in the library at some point.
> 
> But the deadlock is not mentioned in the commit log and lockdep
> doesn't always mean deadlock.

... improve the commit message, reporting the real deadlock case
instead of a lockdep warning and...

[...]

> > > > @@ -207,7 +224,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 = atomic_read(&lpi2c_imx->rate_per);
> > > > 
> > > >  	if (!clk_rate)
> > > >  	
> > > >  		return -EINVAL;
> > > 
> > > Doesn't seem like EINVAL, defined as "Invalid argument", is the
> > > correct number here. As we are failing to read the clock rate, do
> > > you think EIO is better?
> > 
> > Well, this is already the current error code. In both the old and new code I 
> > would consider this error case as 'no clock rate provided' rather than failing 
> > to read. I would reject EIO as there is no IO transfer for retrieving the 
> > clock rate.
> 
> It's definitely not EINVAL as we are failing not because of
> invalid arguments. I thought of EIO because at some point the
> clock rate has been retrieved (or set, thus i/o) and "rate_per"
> updated accordingly. But I agree that's not the perfect value
> either.
> 
> I couldn't think of a better error value, though.

... find a more appropriate error number, I will ack this patch.

If the deadlock you mentioned is a warning from the lockdep, then
please remove the "Fixes:" tag.

Andi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 1/1] i2c: lpi2c: use clk notifier for rate changes

[Alexander Stein]

Hi Andi,

Am Freitag, 10. November 2023, 13:27:20 CET schrieb Andi Shyti:
> Alexander,
> 
> if you...
> 
> On Thu, Nov 09, 2023 at 10:10:46AM +0100, Andi Shyti wrote:
> > On Thu, Nov 09, 2023 at 08:54:51AM +0100, Alexander Stein wrote:
> > > Am Donnerstag, 9. November 2023, 00:38:09 CET schrieb Andi Shyti:
> > > > On Tue, Nov 07, 2023 at 03:12:01PM +0100, Alexander Stein wrote:
> > > > > Instead of repeatedly calling clk_get_rate for each transfer,
> > > > > register
> > > > > a clock notifier to update the cached divider value each time the
> > > > > clock
> > > > > rate actually changes.
> > > > > There is also a lockdep splat if a I2C based clock provider needs to
> > > > > access the i2c bus while in recalc_rate(). "prepare_lock" is about
> > > > > to
> > > > > be locked recursively.
> > > > > 
> > > > > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> > > > 
> > > > What's the exact fix here? Is it the lockdep warning? Is it
> > > > actually causing a real deadlock?
> > > 
> > > We've seen actual deadlocks on our imx8qxp based hardware using a
> > > downstream kernel, so we have implemented as similar fix [1]. This
> > > happened using tlv320aic32x4 audio codec.
> > > The backtrace is similar, a i2c based clock provider is trying t issue
> > > an i2c transfer while adding the clock, thus 'prepare_lock' is already
> > > locked. Lockdep raises an error both for downstream kernel as well as
> > > mainline, both for tlv320aic32x4 or pcf85063.
> > 
> > yes, if the clock is using the same controller then it's likely
> > that you might end up in a deadlock. This is why I like this
> > patch and I believe this shouild go in the library at some point.
> > 
> > But the deadlock is not mentioned in the commit log and lockdep
> > doesn't always mean deadlock.
> 
> ... improve the commit message, reporting the real deadlock case
> instead of a lockdep warning and...

I've improved the commit message about an actual deadlock.

> [...]
> 
> > > > > @@ -207,7 +224,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 = atomic_read(&lpi2c_imx->rate_per);
> > > > > 
> > > > >  	if (!clk_rate)
> > > > >  	
> > > > >  		return -EINVAL;
> > > > 
> > > > Doesn't seem like EINVAL, defined as "Invalid argument", is the
> > > > correct number here. As we are failing to read the clock rate, do
> > > > you think EIO is better?
> > > 
> > > Well, this is already the current error code. In both the old and new
> > > code I would consider this error case as 'no clock rate provided'
> > > rather than failing to read. I would reject EIO as there is no IO
> > > transfer for retrieving the clock rate.
> > 
> > It's definitely not EINVAL as we are failing not because of
> > invalid arguments. I thought of EIO because at some point the
> > clock rate has been retrieved (or set, thus i/o) and "rate_per"
> > updated accordingly. But I agree that's not the perfect value
> > either.
> > 
> > I couldn't think of a better error value, though.
> 
> ... find a more appropriate error number, I will ack this patch.

Thinking about this again, I think EINVAL is an appropriate error code.
The parent clock frequency is also an input for the i2c transfer. So if, for 
whatever reason, that clock frequency is 0, it is an invalid value (argument).
I've checked other drivers what they do if that clock is 0. Unfortunately most 
don't consider this case at all. But some do, so e.g. i2c_lpc2k_probe() or 
dc_i2c_init_hw() both return EINVAL if the clk or a calculated divider is 0.

> If the deadlock you mentioned is a warning from the lockdep, then
> please remove the "Fixes:" tag.

It's not just a lockdep warning, the deadlock actually happened.

Best regards,
Alexander

> Andi


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 1/1] i2c: lpi2c: use clk notifier for rate changes

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 915 bytes --]

Hi,

> > ... improve the commit message, reporting the real deadlock case
> > instead of a lockdep warning and...
> 
> I've improved the commit message about an actual deadlock.

That means a v8 is in your queue?

> > ... find a more appropriate error number, I will ack this patch.
> 
> Thinking about this again, I think EINVAL is an appropriate error code.
> The parent clock frequency is also an input for the i2c transfer. So if, for 
> whatever reason, that clock frequency is 0, it is an invalid value (argument).
> I've checked other drivers what they do if that clock is 0. Unfortunately most 
> don't consider this case at all. But some do, so e.g. i2c_lpc2k_probe() or 
> dc_i2c_init_hw() both return EINVAL if the clk or a calculated divider is 0.

IMHO, the return value doesn't matter that much as we have a descriptive
text accompanying it anyhow.

Happy hacking,

   Wolfram

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel