All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] i2c: mediatek: Optimize master_xfer() and avoid circular locking
@ 2022-04-11 13:21 ` AngeloGioacchino Del Regno
  0 siblings, 0 replies; 12+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-04-11 13:21 UTC (permalink / raw)
  To: qii.wang
  Cc: matthias.bgg, linux-i2c, linux-arm-kernel, linux-mediatek,
	linux-kernel, wsa, nfraprado, kernel, AngeloGioacchino Del Regno

Especially (but not only) during probe, it may happen that multiple
devices are communicating via i2c (or multiple i2c busses) and
sometimes while others are probing asynchronously.
For example, a Cr50 TPM may be filling entropy (or userspace may be
reading random data) while the rt5682 (i2c) codec driver reads/sets
some registers, like while getting/setting a clock's rate, which
happens both during probe and during system operation.

In this driver, the mtk_i2c_transfer() function (which is the i2c
.master_xfer() callback) was granularly managing the clocks by
performing a clk_bulk_prepare_enable() to start them and its inverse.
This is not only creating possible circular locking dependencies in
the some cases (like former explanation), but it's also suboptimal,
as clk_core prepare/unprepare operations are using mutex locking,
which creates a bit of unwanted overhead (for example, i2c trackpads
will call master_xfer() every few milliseconds!).

With this commit, we avoid both the circular locking and additional
overhead by changing how we handle the clocks in this driver:
- Prepare the clocks during probe (and PM resume)
- Enable/disable clocks in mtk_i2c_transfer()
- Unprepare the clocks only for driver removal (and PM suspend)

For the sake of providing a full explanation: during probe, the
clocks are not only prepared but also enabled, as this is needed
for some hardware initialization but, after that, we are disabling
but not unpreparing them, leaving an expected state for the
aforementioned clock handling strategy.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---

v2: Fixed typos in commit description

 drivers/i2c/busses/i2c-mt65xx.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index f651d3e124d6..bdecb78bfc26 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -1177,7 +1177,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
 	int left_num = num;
 	struct mtk_i2c *i2c = i2c_get_adapdata(adap);
 
-	ret = clk_bulk_prepare_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	ret = clk_bulk_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
 	if (ret)
 		return ret;
 
@@ -1231,7 +1231,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
 	ret = num;
 
 err_exit:
-	clk_bulk_disable_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
 	return ret;
 }
 
@@ -1412,7 +1412,7 @@ static int mtk_i2c_probe(struct platform_device *pdev)
 		return ret;
 	}
 	mtk_i2c_init_hw(i2c);
-	clk_bulk_disable_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
 
 	ret = devm_request_irq(&pdev->dev, irq, mtk_i2c_irq,
 			       IRQF_NO_SUSPEND | IRQF_TRIGGER_NONE,
@@ -1439,6 +1439,8 @@ static int mtk_i2c_remove(struct platform_device *pdev)
 
 	i2c_del_adapter(&i2c->adap);
 
+	clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
+
 	return 0;
 }
 
@@ -1448,6 +1450,7 @@ static int mtk_i2c_suspend_noirq(struct device *dev)
 	struct mtk_i2c *i2c = dev_get_drvdata(dev);
 
 	i2c_mark_adapter_suspended(&i2c->adap);
+	clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
 
 	return 0;
 }
@@ -1465,7 +1468,7 @@ static int mtk_i2c_resume_noirq(struct device *dev)
 
 	mtk_i2c_init_hw(i2c);
 
-	clk_bulk_disable_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
 
 	i2c_mark_adapter_resumed(&i2c->adap);
 
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2] i2c: mediatek: Optimize master_xfer() and avoid circular locking
@ 2022-04-11 13:21 ` AngeloGioacchino Del Regno
  0 siblings, 0 replies; 12+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-04-11 13:21 UTC (permalink / raw)
  To: qii.wang
  Cc: matthias.bgg, linux-i2c, linux-arm-kernel, linux-mediatek,
	linux-kernel, wsa, nfraprado, kernel, AngeloGioacchino Del Regno

Especially (but not only) during probe, it may happen that multiple
devices are communicating via i2c (or multiple i2c busses) and
sometimes while others are probing asynchronously.
For example, a Cr50 TPM may be filling entropy (or userspace may be
reading random data) while the rt5682 (i2c) codec driver reads/sets
some registers, like while getting/setting a clock's rate, which
happens both during probe and during system operation.

In this driver, the mtk_i2c_transfer() function (which is the i2c
.master_xfer() callback) was granularly managing the clocks by
performing a clk_bulk_prepare_enable() to start them and its inverse.
This is not only creating possible circular locking dependencies in
the some cases (like former explanation), but it's also suboptimal,
as clk_core prepare/unprepare operations are using mutex locking,
which creates a bit of unwanted overhead (for example, i2c trackpads
will call master_xfer() every few milliseconds!).

With this commit, we avoid both the circular locking and additional
overhead by changing how we handle the clocks in this driver:
- Prepare the clocks during probe (and PM resume)
- Enable/disable clocks in mtk_i2c_transfer()
- Unprepare the clocks only for driver removal (and PM suspend)

For the sake of providing a full explanation: during probe, the
clocks are not only prepared but also enabled, as this is needed
for some hardware initialization but, after that, we are disabling
but not unpreparing them, leaving an expected state for the
aforementioned clock handling strategy.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---

v2: Fixed typos in commit description

 drivers/i2c/busses/i2c-mt65xx.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index f651d3e124d6..bdecb78bfc26 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -1177,7 +1177,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
 	int left_num = num;
 	struct mtk_i2c *i2c = i2c_get_adapdata(adap);
 
-	ret = clk_bulk_prepare_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	ret = clk_bulk_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
 	if (ret)
 		return ret;
 
@@ -1231,7 +1231,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
 	ret = num;
 
 err_exit:
-	clk_bulk_disable_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
 	return ret;
 }
 
@@ -1412,7 +1412,7 @@ static int mtk_i2c_probe(struct platform_device *pdev)
 		return ret;
 	}
 	mtk_i2c_init_hw(i2c);
-	clk_bulk_disable_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
 
 	ret = devm_request_irq(&pdev->dev, irq, mtk_i2c_irq,
 			       IRQF_NO_SUSPEND | IRQF_TRIGGER_NONE,
@@ -1439,6 +1439,8 @@ static int mtk_i2c_remove(struct platform_device *pdev)
 
 	i2c_del_adapter(&i2c->adap);
 
+	clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
+
 	return 0;
 }
 
@@ -1448,6 +1450,7 @@ static int mtk_i2c_suspend_noirq(struct device *dev)
 	struct mtk_i2c *i2c = dev_get_drvdata(dev);
 
 	i2c_mark_adapter_suspended(&i2c->adap);
+	clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
 
 	return 0;
 }
@@ -1465,7 +1468,7 @@ static int mtk_i2c_resume_noirq(struct device *dev)
 
 	mtk_i2c_init_hw(i2c);
 
-	clk_bulk_disable_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
 
 	i2c_mark_adapter_resumed(&i2c->adap);
 
-- 
2.35.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2] i2c: mediatek: Optimize master_xfer() and avoid circular locking
@ 2022-04-11 13:21 ` AngeloGioacchino Del Regno
  0 siblings, 0 replies; 12+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-04-11 13:21 UTC (permalink / raw)
  To: qii.wang
  Cc: matthias.bgg, linux-i2c, linux-arm-kernel, linux-mediatek,
	linux-kernel, wsa, nfraprado, kernel, AngeloGioacchino Del Regno

Especially (but not only) during probe, it may happen that multiple
devices are communicating via i2c (or multiple i2c busses) and
sometimes while others are probing asynchronously.
For example, a Cr50 TPM may be filling entropy (or userspace may be
reading random data) while the rt5682 (i2c) codec driver reads/sets
some registers, like while getting/setting a clock's rate, which
happens both during probe and during system operation.

In this driver, the mtk_i2c_transfer() function (which is the i2c
.master_xfer() callback) was granularly managing the clocks by
performing a clk_bulk_prepare_enable() to start them and its inverse.
This is not only creating possible circular locking dependencies in
the some cases (like former explanation), but it's also suboptimal,
as clk_core prepare/unprepare operations are using mutex locking,
which creates a bit of unwanted overhead (for example, i2c trackpads
will call master_xfer() every few milliseconds!).

With this commit, we avoid both the circular locking and additional
overhead by changing how we handle the clocks in this driver:
- Prepare the clocks during probe (and PM resume)
- Enable/disable clocks in mtk_i2c_transfer()
- Unprepare the clocks only for driver removal (and PM suspend)

For the sake of providing a full explanation: during probe, the
clocks are not only prepared but also enabled, as this is needed
for some hardware initialization but, after that, we are disabling
but not unpreparing them, leaving an expected state for the
aforementioned clock handling strategy.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---

v2: Fixed typos in commit description

 drivers/i2c/busses/i2c-mt65xx.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index f651d3e124d6..bdecb78bfc26 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -1177,7 +1177,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
 	int left_num = num;
 	struct mtk_i2c *i2c = i2c_get_adapdata(adap);
 
-	ret = clk_bulk_prepare_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	ret = clk_bulk_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
 	if (ret)
 		return ret;
 
@@ -1231,7 +1231,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
 	ret = num;
 
 err_exit:
-	clk_bulk_disable_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
 	return ret;
 }
 
@@ -1412,7 +1412,7 @@ static int mtk_i2c_probe(struct platform_device *pdev)
 		return ret;
 	}
 	mtk_i2c_init_hw(i2c);
-	clk_bulk_disable_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
 
 	ret = devm_request_irq(&pdev->dev, irq, mtk_i2c_irq,
 			       IRQF_NO_SUSPEND | IRQF_TRIGGER_NONE,
@@ -1439,6 +1439,8 @@ static int mtk_i2c_remove(struct platform_device *pdev)
 
 	i2c_del_adapter(&i2c->adap);
 
+	clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
+
 	return 0;
 }
 
@@ -1448,6 +1450,7 @@ static int mtk_i2c_suspend_noirq(struct device *dev)
 	struct mtk_i2c *i2c = dev_get_drvdata(dev);
 
 	i2c_mark_adapter_suspended(&i2c->adap);
+	clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
 
 	return 0;
 }
@@ -1465,7 +1468,7 @@ static int mtk_i2c_resume_noirq(struct device *dev)
 
 	mtk_i2c_init_hw(i2c);
 
-	clk_bulk_disable_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
 
 	i2c_mark_adapter_resumed(&i2c->adap);
 
-- 
2.35.1


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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] i2c: mediatek: Optimize master_xfer() and avoid circular locking
  2022-04-11 13:21 ` AngeloGioacchino Del Regno
  (?)
@ 2022-04-21 10:53   ` AngeloGioacchino Del Regno
  -1 siblings, 0 replies; 12+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-04-21 10:53 UTC (permalink / raw)
  To: qii.wang
  Cc: matthias.bgg, linux-i2c, linux-arm-kernel, linux-mediatek,
	linux-kernel, wsa, nfraprado, kernel

Il 11/04/22 15:21, AngeloGioacchino Del Regno ha scritto:
> Especially (but not only) during probe, it may happen that multiple
> devices are communicating via i2c (or multiple i2c busses) and
> sometimes while others are probing asynchronously.
> For example, a Cr50 TPM may be filling entropy (or userspace may be
> reading random data) while the rt5682 (i2c) codec driver reads/sets
> some registers, like while getting/setting a clock's rate, which
> happens both during probe and during system operation.
> 
> In this driver, the mtk_i2c_transfer() function (which is the i2c
> .master_xfer() callback) was granularly managing the clocks by
> performing a clk_bulk_prepare_enable() to start them and its inverse.
> This is not only creating possible circular locking dependencies in
> the some cases (like former explanation), but it's also suboptimal,
> as clk_core prepare/unprepare operations are using mutex locking,
> which creates a bit of unwanted overhead (for example, i2c trackpads
> will call master_xfer() every few milliseconds!).
> 
> With this commit, we avoid both the circular locking and additional
> overhead by changing how we handle the clocks in this driver:
> - Prepare the clocks during probe (and PM resume)
> - Enable/disable clocks in mtk_i2c_transfer()
> - Unprepare the clocks only for driver removal (and PM suspend)
> 
> For the sake of providing a full explanation: during probe, the
> clocks are not only prepared but also enabled, as this is needed
> for some hardware initialization but, after that, we are disabling
> but not unpreparing them, leaving an expected state for the
> aforementioned clock handling strategy.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Hello,
this is a friendly ping to not let this be forgotten.

Cheers,
Angelo

> ---
> 
> v2: Fixed typos in commit description
> 
>   drivers/i2c/busses/i2c-mt65xx.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] i2c: mediatek: Optimize master_xfer() and avoid circular locking
@ 2022-04-21 10:53   ` AngeloGioacchino Del Regno
  0 siblings, 0 replies; 12+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-04-21 10:53 UTC (permalink / raw)
  To: qii.wang
  Cc: matthias.bgg, linux-i2c, linux-arm-kernel, linux-mediatek,
	linux-kernel, wsa, nfraprado, kernel

Il 11/04/22 15:21, AngeloGioacchino Del Regno ha scritto:
> Especially (but not only) during probe, it may happen that multiple
> devices are communicating via i2c (or multiple i2c busses) and
> sometimes while others are probing asynchronously.
> For example, a Cr50 TPM may be filling entropy (or userspace may be
> reading random data) while the rt5682 (i2c) codec driver reads/sets
> some registers, like while getting/setting a clock's rate, which
> happens both during probe and during system operation.
> 
> In this driver, the mtk_i2c_transfer() function (which is the i2c
> .master_xfer() callback) was granularly managing the clocks by
> performing a clk_bulk_prepare_enable() to start them and its inverse.
> This is not only creating possible circular locking dependencies in
> the some cases (like former explanation), but it's also suboptimal,
> as clk_core prepare/unprepare operations are using mutex locking,
> which creates a bit of unwanted overhead (for example, i2c trackpads
> will call master_xfer() every few milliseconds!).
> 
> With this commit, we avoid both the circular locking and additional
> overhead by changing how we handle the clocks in this driver:
> - Prepare the clocks during probe (and PM resume)
> - Enable/disable clocks in mtk_i2c_transfer()
> - Unprepare the clocks only for driver removal (and PM suspend)
> 
> For the sake of providing a full explanation: during probe, the
> clocks are not only prepared but also enabled, as this is needed
> for some hardware initialization but, after that, we are disabling
> but not unpreparing them, leaving an expected state for the
> aforementioned clock handling strategy.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Hello,
this is a friendly ping to not let this be forgotten.

Cheers,
Angelo

> ---
> 
> v2: Fixed typos in commit description
> 
>   drivers/i2c/busses/i2c-mt65xx.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] i2c: mediatek: Optimize master_xfer() and avoid circular locking
@ 2022-04-21 10:53   ` AngeloGioacchino Del Regno
  0 siblings, 0 replies; 12+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-04-21 10:53 UTC (permalink / raw)
  To: qii.wang
  Cc: matthias.bgg, linux-i2c, linux-arm-kernel, linux-mediatek,
	linux-kernel, wsa, nfraprado, kernel

Il 11/04/22 15:21, AngeloGioacchino Del Regno ha scritto:
> Especially (but not only) during probe, it may happen that multiple
> devices are communicating via i2c (or multiple i2c busses) and
> sometimes while others are probing asynchronously.
> For example, a Cr50 TPM may be filling entropy (or userspace may be
> reading random data) while the rt5682 (i2c) codec driver reads/sets
> some registers, like while getting/setting a clock's rate, which
> happens both during probe and during system operation.
> 
> In this driver, the mtk_i2c_transfer() function (which is the i2c
> .master_xfer() callback) was granularly managing the clocks by
> performing a clk_bulk_prepare_enable() to start them and its inverse.
> This is not only creating possible circular locking dependencies in
> the some cases (like former explanation), but it's also suboptimal,
> as clk_core prepare/unprepare operations are using mutex locking,
> which creates a bit of unwanted overhead (for example, i2c trackpads
> will call master_xfer() every few milliseconds!).
> 
> With this commit, we avoid both the circular locking and additional
> overhead by changing how we handle the clocks in this driver:
> - Prepare the clocks during probe (and PM resume)
> - Enable/disable clocks in mtk_i2c_transfer()
> - Unprepare the clocks only for driver removal (and PM suspend)
> 
> For the sake of providing a full explanation: during probe, the
> clocks are not only prepared but also enabled, as this is needed
> for some hardware initialization but, after that, we are disabling
> but not unpreparing them, leaving an expected state for the
> aforementioned clock handling strategy.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Hello,
this is a friendly ping to not let this be forgotten.

Cheers,
Angelo

> ---
> 
> v2: Fixed typos in commit description
> 
>   drivers/i2c/busses/i2c-mt65xx.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] i2c: mediatek: Optimize master_xfer() and avoid circular locking
  2022-04-11 13:21 ` AngeloGioacchino Del Regno
  (?)
@ 2022-04-22  0:21   ` Qii Wang
  -1 siblings, 0 replies; 12+ messages in thread
From: Qii Wang @ 2022-04-22  0:21 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: matthias.bgg, linux-i2c, linux-arm-kernel, linux-mediatek,
	linux-kernel, wsa, nfraprado, kernel

On Mon, 2022-04-11 at 15:21 +0200, AngeloGioacchino Del Regno wrote:
> Especially (but not only) during probe, it may happen that multiple
> devices are communicating via i2c (or multiple i2c busses) and
> sometimes while others are probing asynchronously.
> For example, a Cr50 TPM may be filling entropy (or userspace may be
> reading random data) while the rt5682 (i2c) codec driver reads/sets
> some registers, like while getting/setting a clock's rate, which
> happens both during probe and during system operation.
> 
> In this driver, the mtk_i2c_transfer() function (which is the i2c
> .master_xfer() callback) was granularly managing the clocks by
> performing a clk_bulk_prepare_enable() to start them and its inverse.
> This is not only creating possible circular locking dependencies in
> the some cases (like former explanation), but it's also suboptimal,
> as clk_core prepare/unprepare operations are using mutex locking,
> which creates a bit of unwanted overhead (for example, i2c trackpads
> will call master_xfer() every few milliseconds!).
> 
> With this commit, we avoid both the circular locking and additional
> overhead by changing how we handle the clocks in this driver:
> - Prepare the clocks during probe (and PM resume)
> - Enable/disable clocks in mtk_i2c_transfer()
> - Unprepare the clocks only for driver removal (and PM suspend)
> 
> For the sake of providing a full explanation: during probe, the
> clocks are not only prepared but also enabled, as this is needed
> for some hardware initialization but, after that, we are disabling
> but not unpreparing them, leaving an expected state for the
> aforementioned clock handling strategy.
> 
> Signed-off-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Reviewed-by: Qii Wang <qii.wang@mediatek.com>

> ---
> 
> v2: Fixed typos in commit description
> 
>  drivers/i2c/busses/i2c-mt65xx.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] i2c: mediatek: Optimize master_xfer() and avoid circular locking
@ 2022-04-22  0:21   ` Qii Wang
  0 siblings, 0 replies; 12+ messages in thread
From: Qii Wang @ 2022-04-22  0:21 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: matthias.bgg, linux-i2c, linux-arm-kernel, linux-mediatek,
	linux-kernel, wsa, nfraprado, kernel

On Mon, 2022-04-11 at 15:21 +0200, AngeloGioacchino Del Regno wrote:
> Especially (but not only) during probe, it may happen that multiple
> devices are communicating via i2c (or multiple i2c busses) and
> sometimes while others are probing asynchronously.
> For example, a Cr50 TPM may be filling entropy (or userspace may be
> reading random data) while the rt5682 (i2c) codec driver reads/sets
> some registers, like while getting/setting a clock's rate, which
> happens both during probe and during system operation.
> 
> In this driver, the mtk_i2c_transfer() function (which is the i2c
> .master_xfer() callback) was granularly managing the clocks by
> performing a clk_bulk_prepare_enable() to start them and its inverse.
> This is not only creating possible circular locking dependencies in
> the some cases (like former explanation), but it's also suboptimal,
> as clk_core prepare/unprepare operations are using mutex locking,
> which creates a bit of unwanted overhead (for example, i2c trackpads
> will call master_xfer() every few milliseconds!).
> 
> With this commit, we avoid both the circular locking and additional
> overhead by changing how we handle the clocks in this driver:
> - Prepare the clocks during probe (and PM resume)
> - Enable/disable clocks in mtk_i2c_transfer()
> - Unprepare the clocks only for driver removal (and PM suspend)
> 
> For the sake of providing a full explanation: during probe, the
> clocks are not only prepared but also enabled, as this is needed
> for some hardware initialization but, after that, we are disabling
> but not unpreparing them, leaving an expected state for the
> aforementioned clock handling strategy.
> 
> Signed-off-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Reviewed-by: Qii Wang <qii.wang@mediatek.com>

> ---
> 
> v2: Fixed typos in commit description
> 
>  drivers/i2c/busses/i2c-mt65xx.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> 
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] i2c: mediatek: Optimize master_xfer() and avoid circular locking
@ 2022-04-22  0:21   ` Qii Wang
  0 siblings, 0 replies; 12+ messages in thread
From: Qii Wang @ 2022-04-22  0:21 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: matthias.bgg, linux-i2c, linux-arm-kernel, linux-mediatek,
	linux-kernel, wsa, nfraprado, kernel

On Mon, 2022-04-11 at 15:21 +0200, AngeloGioacchino Del Regno wrote:
> Especially (but not only) during probe, it may happen that multiple
> devices are communicating via i2c (or multiple i2c busses) and
> sometimes while others are probing asynchronously.
> For example, a Cr50 TPM may be filling entropy (or userspace may be
> reading random data) while the rt5682 (i2c) codec driver reads/sets
> some registers, like while getting/setting a clock's rate, which
> happens both during probe and during system operation.
> 
> In this driver, the mtk_i2c_transfer() function (which is the i2c
> .master_xfer() callback) was granularly managing the clocks by
> performing a clk_bulk_prepare_enable() to start them and its inverse.
> This is not only creating possible circular locking dependencies in
> the some cases (like former explanation), but it's also suboptimal,
> as clk_core prepare/unprepare operations are using mutex locking,
> which creates a bit of unwanted overhead (for example, i2c trackpads
> will call master_xfer() every few milliseconds!).
> 
> With this commit, we avoid both the circular locking and additional
> overhead by changing how we handle the clocks in this driver:
> - Prepare the clocks during probe (and PM resume)
> - Enable/disable clocks in mtk_i2c_transfer()
> - Unprepare the clocks only for driver removal (and PM suspend)
> 
> For the sake of providing a full explanation: during probe, the
> clocks are not only prepared but also enabled, as this is needed
> for some hardware initialization but, after that, we are disabling
> but not unpreparing them, leaving an expected state for the
> aforementioned clock handling strategy.
> 
> Signed-off-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Reviewed-by: Qii Wang <qii.wang@mediatek.com>

> ---
> 
> v2: Fixed typos in commit description
> 
>  drivers/i2c/busses/i2c-mt65xx.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] i2c: mediatek: Optimize master_xfer() and avoid circular locking
  2022-04-11 13:21 ` AngeloGioacchino Del Regno
  (?)
@ 2022-05-04 20:20   ` Wolfram Sang
  -1 siblings, 0 replies; 12+ messages in thread
From: Wolfram Sang @ 2022-05-04 20:20 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: qii.wang, matthias.bgg, linux-i2c, linux-arm-kernel,
	linux-mediatek, linux-kernel, nfraprado, kernel

[-- Attachment #1: Type: text/plain, Size: 1897 bytes --]

On Mon, Apr 11, 2022 at 03:21:07PM +0200, AngeloGioacchino Del Regno wrote:
> Especially (but not only) during probe, it may happen that multiple
> devices are communicating via i2c (or multiple i2c busses) and
> sometimes while others are probing asynchronously.
> For example, a Cr50 TPM may be filling entropy (or userspace may be
> reading random data) while the rt5682 (i2c) codec driver reads/sets
> some registers, like while getting/setting a clock's rate, which
> happens both during probe and during system operation.
> 
> In this driver, the mtk_i2c_transfer() function (which is the i2c
> .master_xfer() callback) was granularly managing the clocks by
> performing a clk_bulk_prepare_enable() to start them and its inverse.
> This is not only creating possible circular locking dependencies in
> the some cases (like former explanation), but it's also suboptimal,
> as clk_core prepare/unprepare operations are using mutex locking,
> which creates a bit of unwanted overhead (for example, i2c trackpads
> will call master_xfer() every few milliseconds!).
> 
> With this commit, we avoid both the circular locking and additional
> overhead by changing how we handle the clocks in this driver:
> - Prepare the clocks during probe (and PM resume)
> - Enable/disable clocks in mtk_i2c_transfer()
> - Unprepare the clocks only for driver removal (and PM suspend)
> 
> For the sake of providing a full explanation: during probe, the
> clocks are not only prepared but also enabled, as this is needed
> for some hardware initialization but, after that, we are disabling
> but not unpreparing them, leaving an expected state for the
> aforementioned clock handling strategy.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Applied to for-next, thanks!


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] i2c: mediatek: Optimize master_xfer() and avoid circular locking
@ 2022-05-04 20:20   ` Wolfram Sang
  0 siblings, 0 replies; 12+ messages in thread
From: Wolfram Sang @ 2022-05-04 20:20 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: qii.wang, matthias.bgg, linux-i2c, linux-arm-kernel,
	linux-mediatek, linux-kernel, nfraprado, kernel


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

On Mon, Apr 11, 2022 at 03:21:07PM +0200, AngeloGioacchino Del Regno wrote:
> Especially (but not only) during probe, it may happen that multiple
> devices are communicating via i2c (or multiple i2c busses) and
> sometimes while others are probing asynchronously.
> For example, a Cr50 TPM may be filling entropy (or userspace may be
> reading random data) while the rt5682 (i2c) codec driver reads/sets
> some registers, like while getting/setting a clock's rate, which
> happens both during probe and during system operation.
> 
> In this driver, the mtk_i2c_transfer() function (which is the i2c
> .master_xfer() callback) was granularly managing the clocks by
> performing a clk_bulk_prepare_enable() to start them and its inverse.
> This is not only creating possible circular locking dependencies in
> the some cases (like former explanation), but it's also suboptimal,
> as clk_core prepare/unprepare operations are using mutex locking,
> which creates a bit of unwanted overhead (for example, i2c trackpads
> will call master_xfer() every few milliseconds!).
> 
> With this commit, we avoid both the circular locking and additional
> overhead by changing how we handle the clocks in this driver:
> - Prepare the clocks during probe (and PM resume)
> - Enable/disable clocks in mtk_i2c_transfer()
> - Unprepare the clocks only for driver removal (and PM suspend)
> 
> For the sake of providing a full explanation: during probe, the
> clocks are not only prepared but also enabled, as this is needed
> for some hardware initialization but, after that, we are disabling
> but not unpreparing them, leaving an expected state for the
> aforementioned clock handling strategy.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Applied to for-next, thanks!


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

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] i2c: mediatek: Optimize master_xfer() and avoid circular locking
@ 2022-05-04 20:20   ` Wolfram Sang
  0 siblings, 0 replies; 12+ messages in thread
From: Wolfram Sang @ 2022-05-04 20:20 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: qii.wang, matthias.bgg, linux-i2c, linux-arm-kernel,
	linux-mediatek, linux-kernel, nfraprado, kernel


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

On Mon, Apr 11, 2022 at 03:21:07PM +0200, AngeloGioacchino Del Regno wrote:
> Especially (but not only) during probe, it may happen that multiple
> devices are communicating via i2c (or multiple i2c busses) and
> sometimes while others are probing asynchronously.
> For example, a Cr50 TPM may be filling entropy (or userspace may be
> reading random data) while the rt5682 (i2c) codec driver reads/sets
> some registers, like while getting/setting a clock's rate, which
> happens both during probe and during system operation.
> 
> In this driver, the mtk_i2c_transfer() function (which is the i2c
> .master_xfer() callback) was granularly managing the clocks by
> performing a clk_bulk_prepare_enable() to start them and its inverse.
> This is not only creating possible circular locking dependencies in
> the some cases (like former explanation), but it's also suboptimal,
> as clk_core prepare/unprepare operations are using mutex locking,
> which creates a bit of unwanted overhead (for example, i2c trackpads
> will call master_xfer() every few milliseconds!).
> 
> With this commit, we avoid both the circular locking and additional
> overhead by changing how we handle the clocks in this driver:
> - Prepare the clocks during probe (and PM resume)
> - Enable/disable clocks in mtk_i2c_transfer()
> - Unprepare the clocks only for driver removal (and PM suspend)
> 
> For the sake of providing a full explanation: during probe, the
> clocks are not only prepared but also enabled, as this is needed
> for some hardware initialization but, after that, we are disabling
> but not unpreparing them, leaving an expected state for the
> aforementioned clock handling strategy.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Applied to for-next, thanks!


[-- 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-05-04 20:21 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-11 13:21 [PATCH v2] i2c: mediatek: Optimize master_xfer() and avoid circular locking AngeloGioacchino Del Regno
2022-04-11 13:21 ` AngeloGioacchino Del Regno
2022-04-11 13:21 ` AngeloGioacchino Del Regno
2022-04-21 10:53 ` AngeloGioacchino Del Regno
2022-04-21 10:53   ` AngeloGioacchino Del Regno
2022-04-21 10:53   ` AngeloGioacchino Del Regno
2022-04-22  0:21 ` Qii Wang
2022-04-22  0:21   ` Qii Wang
2022-04-22  0:21   ` Qii Wang
2022-05-04 20:20 ` Wolfram Sang
2022-05-04 20:20   ` Wolfram Sang
2022-05-04 20:20   ` Wolfram Sang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.