diff mbox series

[v5,1/1] i2c: mediatek: add runtime PM operations and bus regulator control

Message ID 20250314145407.2900190-1-zoie.lin@mediatek.com
State New
Headers show
Series [v5,1/1] i2c: mediatek: add runtime PM operations and bus regulator control | expand

Commit Message

Zoie Lin March 14, 2025, 2:53 p.m. UTC 
Introduce support for runtime PM operations in
the I2C driver, enabling runtime suspend and resume functionality.

Although in most platforms, the bus power of i2c is always
on, some platforms disable the i2c bus power in order to meet
low power request.

This implementation includes bus regulator control to facilitate
proper handling of the bus power based on platform requirements.

Signed-off-by: Zoie Lin <zoie.lin@mediatek.com>
---
 drivers/i2c/busses/i2c-mt65xx.c | 74 ++++++++++++++++++++++++++++-----
 1 file changed, 63 insertions(+), 11 deletions(-)

This series is based on linux-next, tag: next-20250314

Changes in v5:
- Add pm runtime autosuspend setting to 250ms
- Add error message for clock enable failure

Changes in v4:
- Removed unnecessary variable initialization.
- Removed unnecessary brackets.
- Corrected grammar issues in the commit message.
- Confirmed autosuspend delay is not necessary.

Changes in v3:
- Removed the autosuspend functionality
- Resumed the I2C bus in mtk_i2c_resume_noirq() 

Changes in v2:
- Author name modification
- Replacement of pm runtime API
- Removal of redundant error messages
- Return value adjustment
- Add runtime pm status check

Comments

Andi Shyti March 18, 2025, 11:34 p.m. UTC | #1 
Hi Zoie,

On Fri, Mar 14, 2025 at 10:53:43PM +0800, Zoie Lin wrote:
> Introduce support for runtime PM operations in
> the I2C driver, enabling runtime suspend and resume functionality.
> 
> Although in most platforms, the bus power of i2c is always
> on, some platforms disable the i2c bus power in order to meet
> low power request.
> 
> This implementation includes bus regulator control to facilitate
> proper handling of the bus power based on platform requirements.
> 
> Signed-off-by: Zoie Lin <zoie.lin@mediatek.com>

merged to i2c/i2c-host.

Thanks,
Andi
Daniel Golle March 20, 2025, 10:08 p.m. UTC | #2 
Hi Andi,
Hi Zoie,

On Wed, Mar 19, 2025 at 12:34:53AM +0100, Andi Shyti wrote:
> Hi Zoie,
> 
> On Fri, Mar 14, 2025 at 10:53:43PM +0800, Zoie Lin wrote:
> > Introduce support for runtime PM operations in
> > the I2C driver, enabling runtime suspend and resume functionality.
> > 
> > Although in most platforms, the bus power of i2c is always
> > on, some platforms disable the i2c bus power in order to meet
> > low power request.
> > 
> > This implementation includes bus regulator control to facilitate
> > proper handling of the bus power based on platform requirements.
> > 
> > Signed-off-by: Zoie Lin <zoie.lin@mediatek.com>
> 
> merged to i2c/i2c-host.

the change causes a crash during boot on MT7988 which typically uses
one of its I2C busses to connect a Richtek RT5190A PMIC.

[    1.465158] SError Interrupt on CPU3, code 0x00000000bf000002 -- SError
[    1.465170] CPU: 3 UID: 0 PID: 40 Comm: kworker/u16:1 Not tainted 6.14.0-rc7-next-20250320+ #0 NONE 
[    1.465178] Hardware name: SmartRG SDG-9000 (DT)
[    1.465180] Workqueue: async async_run_entry_fn
[    1.465194] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    1.465199] pc : mtk_i2c_transfer+0x27c/0x1108
[    1.465208] lr : mtk_i2c_transfer+0x224/0x1108
[    1.465212] sp : ffffffc0821538e0
[    1.465213] x29: ffffffc0821538e0 x28: 0000000000000000 x27: ffffffc082153a00
[    1.465220] x26: 00000000ffff8b62 x25: ffffffc080f3b000 x24: 0000000000000000
[    1.465225] x23: 0000000000000001 x22: ffffff80c0ba7810 x21: 000000000000000f
[    1.465230] x20: ffffffc082153a10 x19: ffffff80c9620080 x18: 0000000000000014
[    1.465236] x17: 000000005f52c058 x16: 00000000c071d659 x15: 0000000013b9e9aa
[    1.465241] x14: 00000000305bde35 x13: ffffff80c5f222e8 x12: ffffff80c5f222b0
[    1.465246] x11: 0000000000001e20 x10: ffffffc0811097d8 x9 : 0000000000000001
[    1.465251] x8 : ffffffc082153830 x7 : ffffffc0821538a0 x6 : ffffffc082153840
[    1.465256] x5 : 000000000000ffed x4 : 0000000000000003 x3 : ffffffc080b798b0
[    1.465260] x2 : 0000000000000000 x1 : ffffffc080b797d8 x0 : 0000000000000064
[    1.465267] Kernel panic - not syncing: Asynchronous SError Interrupt
[    1.465269] SMP: stopping secondary CPUs
[    1.465281] Kernel Offset: disabled
[    1.465282] CPU features: 0x0000,00000040,00000000,8200400b
[    1.465286] Memory Limit: none
[    1.466353] 
[    1.466355] ================================
[    1.466356] WARNING: inconsistent lock state
[    1.466357] 6.14.0-rc7-next-20250320+ #0 Not tainted
[    1.466360] --------------------------------
[    1.466361] inconsistent {INITIAL USE} -> {IN-NMI} usage.
[    1.466364] kworker/u16:1/40 [HC1[1]:SC0[0]:HE0:SE1] takes:
[    1.466369] ffffff80c0357940 (&prz->buffer_lock){....}-{2:2}, at: buffer_size_add+0x68/0x84
[    1.466384] {INITIAL USE} state was registered at:
[    1.466386]   lock_acquire+0xf4/0x2d8
[    1.466391]   _raw_spin_lock_irqsave+0x5c/0x80
[    1.466399]   buffer_size_add+0x68/0x84
[    1.466404]   persistent_ram_write+0x44/0x11c
[    1.466410]   ramoops_pstore_write+0x150/0x1c4
[    1.466415]   pstore_console_write+0x4c/0x5c
[    1.466420]   console_flush_all+0x2ac/0x490
[    1.466425]   console_unlock+0x68/0x144
[    1.466430]   vprintk_emit+0x168/0x3ac
[    1.466434]   vprintk_default+0x34/0x3c
[    1.466439]   vprintk+0x24/0x2c
[    1.466444]   _printk+0x48/0x50
[    1.466448]   register_console+0x370/0x4c4
[    1.466453]   pstore_register+0x18c/0x37c
[    1.466457]   ramoops_probe+0x2e8/0x5c4
[    1.466461]   platform_probe+0x64/0xbc
[    1.466467]   really_probe+0xbc/0x388
[    1.466470]   __driver_probe_device+0x78/0x154
[    1.466474]   driver_probe_device+0x3c/0xd4
[    1.466477]   __device_attach_driver+0xb0/0x144
[    1.466480]   bus_for_each_drv+0x6c/0xb0
[    1.466486]   __device_attach+0x9c/0x19c
[    1.466489]   device_initial_probe+0x10/0x18
[    1.466493]   bus_probe_device+0xa8/0xac
[    1.466495]   device_add+0x58c/0x790
[    1.466500]   of_device_add+0x4c/0x58
[    1.466507]   of_platform_device_create_pdata+0x9c/0x118
[    1.466512]   of_platform_default_populate_init+0x4c/0xf0
[    1.466519]   do_one_initcall+0x64/0x324
[    1.466524]   kernel_init_freeable+0x298/0x4f0
[    1.466530]   kernel_init+0x1c/0x1c8
[    1.466534]   ret_from_fork+0x10/0x20
[    1.466538] irq event stamp: 3498
[    1.466539] hardirqs last  enabled at (3497): [<ffffffc080ac0b78>] _raw_spin_unlock_irqrestore+0x54/0x74
[    1.466546] hardirqs last disabled at (3498): [<ffffffc080ab43c4>] el1h_64_error_handler+0x20/0x48
[    1.466551] softirqs last  enabled at (3392): [<ffffffc08004af28>] handle_softirqs+0x46c/0x484
[    1.466558] softirqs last disabled at (3381): [<ffffffc080010170>] __do_softirq+0x10/0x18
[    1.466562] 
[    1.466562] other info that might help us debug this:
[    1.466564]  Possible unsafe locking scenario:
[    1.466564] 
[    1.466565]        CPU0
[    1.466566]        ----
[    1.466567]   lock(&prz->buffer_lock);
[    1.466570]   <Interrupt>
[    1.466571]     lock(&prz->buffer_lock);
[    1.466573] 
[    1.466573]  *** DEADLOCK ***
[    1.466573] 
[    1.466575] 5 locks held by kworker/u16:1/40:
[    1.466578]  #0: ffffff80c01b2148 ((wq_completion)async){+.+.}-{0:0}, at: process_one_work+0x1a8/0x610
[    1.466588]  #1: ffffffc082153dd8 ((work_completion)(&entry->work)){+.+.}-{0:0}, at: process_one_work+0x1d0/0x610
[    1.466598]  #2: ffffff80c7b3f908 (&dev->mutex){....}-{4:4}, at: __device_attach_async_helper+0x28/0xec
[    1.466606]  #3: ffffff80c5f22068 (rt5190a_regulator:443:(&rt5190a_regmap_config)->lock){+.+.}-{4:4}, at: regmap_lock_mutex+0x10/0x18
[    1.466617]  #4: ffffff80c9620100 (i2c_register_adapter){+.+.}-{4:4}, at: i2c_adapter_lock_bus+0x20/0x2c
[    1.466629] 
[    1.466629] stack backtrace:
[    1.466630] CPU: 3 UID: 0 PID: 40 Comm: kworker/u16:1 Not tainted 6.14.0-rc7-next-20250320+ #0 NONE 
[    1.466635] Hardware name: SmartRG SDG-9000 (DT)
[    1.466637] Workqueue: async async_run_entry_fn
[    1.466643] Call trace:
[    1.466645]  show_stack+0x14/0x1c (C)
[    1.466653]  dump_stack_lvl+0x30/0xc0
[    1.466657]  dump_stack+0x14/0x1c
[    1.466661]  print_usage_bug.part.0+0x26c/0x318
[    1.466665]  lock_acquire+0x280/0x2d8
[    1.466669]  _raw_spin_lock_irqsave+0x5c/0x80
[    1.466676]  buffer_size_add+0x68/0x84
[    1.466681]  persistent_ram_write+0x44/0x11c
[    1.466687]  ramoops_pstore_write+0xc8/0x1c4
[    1.466692]  pstore_dump+0x16c/0x300
[    1.466697]  kmsg_dump_desc+0xc0/0x194
[    1.466703]  panic+0x170/0x38c
[    1.466708]  nmi_panic+0x50/0x64
[    1.466712]  arm64_serror_panic+0x60/0x70
[    1.466715]  do_serror+0x38/0x68
[    1.466719]  el1h_64_error_handler+0x34/0x48
[    1.466723]  el1h_64_error+0x70/0x74
[    1.466727]  mtk_i2c_transfer+0x27c/0x1108 (P)
[    1.466732]  __i2c_transfer+0x1f8/0x948
[    1.466736]  i2c_transfer+0x90/0xe8
[    1.466739]  regmap_i2c_read+0x48/0x70
[    1.466744]  _regmap_raw_read+0x110/0x328
[    1.466750]  regmap_raw_read+0x130/0x240
[    1.466755]  rt5190a_probe+0x88/0x53c
[    1.466761]  i2c_device_probe+0x154/0x2ec
[    1.466764]  really_probe+0xbc/0x388
[    1.466768]  __driver_probe_device+0x78/0x154
[    1.466772]  driver_probe_device+0x3c/0xd4
[    1.466776]  __device_attach_driver+0xb0/0x144
[    1.466780]  bus_for_each_drv+0x6c/0xb0
[    1.466785]  __device_attach_async_helper+0x80/0xec
[    1.466790]  async_run_entry_fn+0x34/0x17c
[    1.466794]  process_one_work+0x224/0x610
[    1.466799]  worker_thread+0x1b4/0x348
[    1.466804]  kthread+0x108/0x1d4
[    1.466808]  ret_from_fork+0x10/0x20

Reverting commit 4b60b1b58696 (i2c: mediatek: add runtime PM operations
and bus regulator control") fixes the issue.
Andi Shyti March 21, 2025, 12:34 a.m. UTC | #3 
Hi Daniel,

On Thu, Mar 20, 2025 at 10:08:42PM +0000, Daniel Golle wrote:
> On Wed, Mar 19, 2025 at 12:34:53AM +0100, Andi Shyti wrote:
> > On Fri, Mar 14, 2025 at 10:53:43PM +0800, Zoie Lin wrote:
> > > Introduce support for runtime PM operations in
> > > the I2C driver, enabling runtime suspend and resume functionality.
> > > 
> > > Although in most platforms, the bus power of i2c is always
> > > on, some platforms disable the i2c bus power in order to meet
> > > low power request.
> > > 
> > > This implementation includes bus regulator control to facilitate
> > > proper handling of the bus power based on platform requirements.
> > > 
> > > Signed-off-by: Zoie Lin <zoie.lin@mediatek.com>
> > 
> > merged to i2c/i2c-host.
> 
> the change causes a crash during boot on MT7988 which typically uses
> one of its I2C busses to connect a Richtek RT5190A PMIC.

Thanks a lot for testing it. I removed the commit.

Andi
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index 5bd342047d59..a630733a8890 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -21,6 +21,7 @@ 
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/scatterlist.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
@@ -1245,8 +1246,8 @@  static int mtk_i2c_transfer(struct i2c_adapter *adap,
 	int left_num = num;
 	struct mtk_i2c *i2c = i2c_get_adapdata(adap);
 
-	ret = clk_bulk_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
-	if (ret)
+	ret = pm_runtime_resume_and_get(i2c->dev);
+	if (ret < 0)
 		return ret;
 
 	i2c->auto_restart = i2c->dev_comp->auto_restart;
@@ -1299,7 +1300,9 @@  static int mtk_i2c_transfer(struct i2c_adapter *adap,
 	ret = num;
 
 err_exit:
-	clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	pm_runtime_mark_last_busy(i2c->dev);
+	pm_runtime_put_autosuspend(i2c->dev);
+
 	return ret;
 }
 
@@ -1370,6 +1373,38 @@  static int mtk_i2c_parse_dt(struct device_node *np, struct mtk_i2c *i2c)
 	return 0;
 }
 
+static int mtk_i2c_runtime_suspend(struct device *dev)
+{
+	struct mtk_i2c *i2c = dev_get_drvdata(dev);
+
+	clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	if (i2c->adap.bus_regulator)
+		regulator_disable(i2c->adap.bus_regulator);
+
+	return 0;
+}
+
+static int mtk_i2c_runtime_resume(struct device *dev)
+{
+	int ret;
+	struct mtk_i2c *i2c = dev_get_drvdata(dev);
+
+	if (i2c->adap.bus_regulator) {
+		ret = regulator_enable(i2c->adap.bus_regulator);
+		if (ret) {
+			dev_err(dev, "enable regulator failed!\n");
+			return ret;
+		}
+	}
+
+	ret = clk_bulk_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	if (ret)
+		if (i2c->adap.bus_regulator)
+			regulator_disable(i2c->adap.bus_regulator);
+
+	return ret;
+}
+
 static int mtk_i2c_probe(struct platform_device *pdev)
 {
 	int ret = 0;
@@ -1472,13 +1507,19 @@  static int mtk_i2c_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = clk_bulk_prepare_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	ret = clk_bulk_prepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
 	if (ret) {
 		dev_err(&pdev->dev, "clock enable failed!\n");
 		return ret;
 	}
+
+	platform_set_drvdata(pdev, i2c);
+
+	ret = mtk_i2c_runtime_resume(i2c->dev);
+	if (ret < 0)
+		goto err_clk_bulk_unprepare;
 	mtk_i2c_init_hw(i2c);
-	clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	mtk_i2c_runtime_suspend(i2c->dev);
 
 	ret = devm_request_irq(&pdev->dev, irq, mtk_i2c_irq,
 			       IRQF_NO_SUSPEND | IRQF_TRIGGER_NONE,
@@ -1486,19 +1527,22 @@  static int mtk_i2c_probe(struct platform_device *pdev)
 	if (ret < 0) {
 		dev_err(&pdev->dev,
 			"Request I2C IRQ %d fail\n", irq);
-		goto err_bulk_unprepare;
+		goto err_clk_bulk_unprepare;
 	}
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 250);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
 
 	i2c_set_adapdata(&i2c->adap, i2c);
 	ret = i2c_add_adapter(&i2c->adap);
 	if (ret)
-		goto err_bulk_unprepare;
-
-	platform_set_drvdata(pdev, i2c);
+		goto err_pm_runtime_disable;
 
 	return 0;
 
-err_bulk_unprepare:
+err_pm_runtime_disable:
+	pm_runtime_disable(&pdev->dev);
+err_clk_bulk_unprepare:
 	clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
 
 	return ret;
@@ -1510,6 +1554,7 @@  static void mtk_i2c_remove(struct platform_device *pdev)
 
 	i2c_del_adapter(&i2c->adap);
 
+	pm_runtime_disable(&pdev->dev);
 	clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
 }
 
@@ -1518,6 +1563,10 @@  static int mtk_i2c_suspend_noirq(struct device *dev)
 	struct mtk_i2c *i2c = dev_get_drvdata(dev);
 
 	i2c_mark_adapter_suspended(&i2c->adap);
+
+	if (!pm_runtime_status_suspended(dev))
+		mtk_i2c_runtime_suspend(dev);
+
 	clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
 
 	return 0;
@@ -1536,7 +1585,8 @@  static int mtk_i2c_resume_noirq(struct device *dev)
 
 	mtk_i2c_init_hw(i2c);
 
-	clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	if (pm_runtime_status_suspended(dev))
+		mtk_i2c_runtime_resume(dev);
 
 	i2c_mark_adapter_resumed(&i2c->adap);
 
@@ -1546,6 +1596,8 @@  static int mtk_i2c_resume_noirq(struct device *dev)
 static const struct dev_pm_ops mtk_i2c_pm = {
 	NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_i2c_suspend_noirq,
 				  mtk_i2c_resume_noirq)
+	SET_RUNTIME_PM_OPS(mtk_i2c_runtime_suspend, mtk_i2c_runtime_resume,
+			   NULL)
 };
 
 static struct platform_driver mtk_i2c_driver = {