* [PATCH 0/4] i2c: designware: runtime pm fix and improve
@ 2016-04-14 12:53 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-14 12:53 UTC (permalink / raw)
To: jarkko.nikula, andriy.shevchenko, mika.westerberg, wsa
Cc: linux-i2c, linux-kernel, linux-arm-kernel, Jisheng Zhang
This series tries to fix one hang issue during probe found on arm
platform, and unbalanced clk enable prepare issue. Then applies similar
change as commit 36d48fb5766a ("i2c: designware-platdrv: enable
RuntimePM before registering to the core"), lastly remove the runtime
suspend prevention in i2c_dw_probe().
Jisheng Zhang (4):
i2c: designware-platdrv: Fix runtime PM initialization
i2c: designware-platdrv: fix unbalanced clk enable and prepare
i2c: designware-pcidrv: enable RuntimePM before registering to the
core
i2c: designware: remove runtime suspend prevention during registration
drivers/i2c/busses/i2c-designware-core.c | 8 --------
drivers/i2c/busses/i2c-designware-pcidrv.c | 11 +++++++----
drivers/i2c/busses/i2c-designware-platdrv.c | 16 +++++++++++++++-
3 files changed, 22 insertions(+), 13 deletions(-)
--
2.8.0.rc3
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 0/4] i2c: designware: runtime pm fix and improve
@ 2016-04-14 12:53 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-14 12:53 UTC (permalink / raw)
To: jarkko.nikula, andriy.shevchenko, mika.westerberg, wsa
Cc: linux-i2c, linux-kernel, linux-arm-kernel, Jisheng Zhang
This series tries to fix one hang issue during probe found on arm
platform, and unbalanced clk enable prepare issue. Then applies similar
change as commit 36d48fb5766a ("i2c: designware-platdrv: enable
RuntimePM before registering to the core"), lastly remove the runtime
suspend prevention in i2c_dw_probe().
Jisheng Zhang (4):
i2c: designware-platdrv: Fix runtime PM initialization
i2c: designware-platdrv: fix unbalanced clk enable and prepare
i2c: designware-pcidrv: enable RuntimePM before registering to the
core
i2c: designware: remove runtime suspend prevention during registration
drivers/i2c/busses/i2c-designware-core.c | 8 --------
drivers/i2c/busses/i2c-designware-pcidrv.c | 11 +++++++----
drivers/i2c/busses/i2c-designware-platdrv.c | 16 +++++++++++++++-
3 files changed, 22 insertions(+), 13 deletions(-)
--
2.8.0.rc3
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 0/4] i2c: designware: runtime pm fix and improve
@ 2016-04-14 12:53 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-14 12:53 UTC (permalink / raw)
To: linux-arm-kernel
This series tries to fix one hang issue during probe found on arm
platform, and unbalanced clk enable prepare issue. Then applies similar
change as commit 36d48fb5766a ("i2c: designware-platdrv: enable
RuntimePM before registering to the core"), lastly remove the runtime
suspend prevention in i2c_dw_probe().
Jisheng Zhang (4):
i2c: designware-platdrv: Fix runtime PM initialization
i2c: designware-platdrv: fix unbalanced clk enable and prepare
i2c: designware-pcidrv: enable RuntimePM before registering to the
core
i2c: designware: remove runtime suspend prevention during registration
drivers/i2c/busses/i2c-designware-core.c | 8 --------
drivers/i2c/busses/i2c-designware-pcidrv.c | 11 +++++++----
drivers/i2c/busses/i2c-designware-platdrv.c | 16 +++++++++++++++-
3 files changed, 22 insertions(+), 13 deletions(-)
--
2.8.0.rc3
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization
2016-04-14 12:53 ` Jisheng Zhang
(?)
@ 2016-04-14 12:53 ` Jisheng Zhang
-1 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-14 12:53 UTC (permalink / raw)
To: jarkko.nikula, andriy.shevchenko, mika.westerberg, wsa
Cc: linux-i2c, linux-kernel, linux-arm-kernel, Jisheng Zhang
When pm_runtime_enable() was being called, the device's usage counter
was 0, causing the PM layer to runtime-suspend the device. We then
went on to call i2c_dw_probe() on a suspended device, which could hung.
Fix this by incrementing the usage counter before pm_runtime_enable().
Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d656657..00f9e99 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
if (dev->pm_runtime_disabled) {
pm_runtime_forbid(&pdev->dev);
} else {
+ pm_runtime_get_noresume(&pdev->dev);
pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_set_active(&pdev->dev);
@@ -253,8 +254,19 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
}
r = i2c_dw_probe(dev);
- if (r && !dev->pm_runtime_disabled)
+ if (r)
+ goto rpm_disable;
+
+ if (!dev->pm_runtime_disabled)
+ pm_runtime_put_autosuspend(&pdev->dev);
+
+ return 0;
+
+rpm_disable:
+ if (!dev->pm_runtime_disabled) {
pm_runtime_disable(&pdev->dev);
+ pm_runtime_put_noidle(&pdev->dev);
+ }
return r;
}
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization
@ 2016-04-14 12:53 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-14 12:53 UTC (permalink / raw)
To: jarkko.nikula, andriy.shevchenko, mika.westerberg, wsa
Cc: linux-i2c, linux-kernel, linux-arm-kernel, Jisheng Zhang
When pm_runtime_enable() was being called, the device's usage counter
was 0, causing the PM layer to runtime-suspend the device. We then
went on to call i2c_dw_probe() on a suspended device, which could hung.
Fix this by incrementing the usage counter before pm_runtime_enable().
Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d656657..00f9e99 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
if (dev->pm_runtime_disabled) {
pm_runtime_forbid(&pdev->dev);
} else {
+ pm_runtime_get_noresume(&pdev->dev);
pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_set_active(&pdev->dev);
@@ -253,8 +254,19 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
}
r = i2c_dw_probe(dev);
- if (r && !dev->pm_runtime_disabled)
+ if (r)
+ goto rpm_disable;
+
+ if (!dev->pm_runtime_disabled)
+ pm_runtime_put_autosuspend(&pdev->dev);
+
+ return 0;
+
+rpm_disable:
+ if (!dev->pm_runtime_disabled) {
pm_runtime_disable(&pdev->dev);
+ pm_runtime_put_noidle(&pdev->dev);
+ }
return r;
}
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization
@ 2016-04-14 12:53 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-14 12:53 UTC (permalink / raw)
To: linux-arm-kernel
When pm_runtime_enable() was being called, the device's usage counter
was 0, causing the PM layer to runtime-suspend the device. We then
went on to call i2c_dw_probe() on a suspended device, which could hung.
Fix this by incrementing the usage counter before pm_runtime_enable().
Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d656657..00f9e99 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
if (dev->pm_runtime_disabled) {
pm_runtime_forbid(&pdev->dev);
} else {
+ pm_runtime_get_noresume(&pdev->dev);
pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_set_active(&pdev->dev);
@@ -253,8 +254,19 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
}
r = i2c_dw_probe(dev);
- if (r && !dev->pm_runtime_disabled)
+ if (r)
+ goto rpm_disable;
+
+ if (!dev->pm_runtime_disabled)
+ pm_runtime_put_autosuspend(&pdev->dev);
+
+ return 0;
+
+rpm_disable:
+ if (!dev->pm_runtime_disabled) {
pm_runtime_disable(&pdev->dev);
+ pm_runtime_put_noidle(&pdev->dev);
+ }
return r;
}
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare
2016-04-14 12:53 ` Jisheng Zhang
(?)
@ 2016-04-14 12:53 ` Jisheng Zhang
-1 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-14 12:53 UTC (permalink / raw)
To: jarkko.nikula, andriy.shevchenko, mika.westerberg, wsa
Cc: linux-i2c, linux-kernel, linux-arm-kernel, Jisheng Zhang
If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
disable and unprepare the clk, otherwise the clk enable and prepare
is left unbalanced.
Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 00f9e99..1488cea 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -268,6 +268,8 @@ rpm_disable:
pm_runtime_put_noidle(&pdev->dev);
}
+ i2c_dw_plat_prepare_clk(dev, false);
+
return r;
}
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare
@ 2016-04-14 12:53 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-14 12:53 UTC (permalink / raw)
To: jarkko.nikula, andriy.shevchenko, mika.westerberg, wsa
Cc: Jisheng Zhang, linux-i2c, linux-arm-kernel, linux-kernel
If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
disable and unprepare the clk, otherwise the clk enable and prepare
is left unbalanced.
Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 00f9e99..1488cea 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -268,6 +268,8 @@ rpm_disable:
pm_runtime_put_noidle(&pdev->dev);
}
+ i2c_dw_plat_prepare_clk(dev, false);
+
return r;
}
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare
@ 2016-04-14 12:53 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-14 12:53 UTC (permalink / raw)
To: linux-arm-kernel
If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
disable and unprepare the clk, otherwise the clk enable and prepare
is left unbalanced.
Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 00f9e99..1488cea 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -268,6 +268,8 @@ rpm_disable:
pm_runtime_put_noidle(&pdev->dev);
}
+ i2c_dw_plat_prepare_clk(dev, false);
+
return r;
}
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 3/4] i2c: designware-pcidrv: enable RuntimePM before registering to the core
2016-04-14 12:53 ` Jisheng Zhang
(?)
@ 2016-04-14 12:53 ` Jisheng Zhang
-1 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-14 12:53 UTC (permalink / raw)
To: jarkko.nikula, andriy.shevchenko, mika.westerberg, wsa
Cc: linux-i2c, linux-kernel, linux-arm-kernel, Jisheng Zhang
As pointed out by commit 36d48fb5766a ("i2c: designware-platdrv: enable
RuntimePM before registering to the core"), "The core may register
clients attached to this master which may use funtionality from the
master", so enable RuntimePM before registering to the core.
Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
drivers/i2c/busses/i2c-designware-pcidrv.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 7368be0..41a01a7 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -248,14 +248,17 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
adap->nr = controller->bus_num;
+ pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_runtime_allow(&pdev->dev);
+
r = i2c_dw_probe(dev);
- if (r)
+ if (r) {
+ pm_runtime_forbid(&pdev->dev);
return r;
+ }
- pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
- pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_put_autosuspend(&pdev->dev);
- pm_runtime_allow(&pdev->dev);
return 0;
}
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 3/4] i2c: designware-pcidrv: enable RuntimePM before registering to the core
@ 2016-04-14 12:53 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-14 12:53 UTC (permalink / raw)
To: jarkko.nikula, andriy.shevchenko, mika.westerberg, wsa
Cc: linux-i2c, linux-kernel, linux-arm-kernel, Jisheng Zhang
As pointed out by commit 36d48fb5766a ("i2c: designware-platdrv: enable
RuntimePM before registering to the core"), "The core may register
clients attached to this master which may use funtionality from the
master", so enable RuntimePM before registering to the core.
Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
drivers/i2c/busses/i2c-designware-pcidrv.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 7368be0..41a01a7 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -248,14 +248,17 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
adap->nr = controller->bus_num;
+ pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_runtime_allow(&pdev->dev);
+
r = i2c_dw_probe(dev);
- if (r)
+ if (r) {
+ pm_runtime_forbid(&pdev->dev);
return r;
+ }
- pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
- pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_put_autosuspend(&pdev->dev);
- pm_runtime_allow(&pdev->dev);
return 0;
}
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 3/4] i2c: designware-pcidrv: enable RuntimePM before registering to the core
@ 2016-04-14 12:53 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-14 12:53 UTC (permalink / raw)
To: linux-arm-kernel
As pointed out by commit 36d48fb5766a ("i2c: designware-platdrv: enable
RuntimePM before registering to the core"), "The core may register
clients attached to this master which may use funtionality from the
master", so enable RuntimePM before registering to the core.
Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
drivers/i2c/busses/i2c-designware-pcidrv.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 7368be0..41a01a7 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -248,14 +248,17 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
adap->nr = controller->bus_num;
+ pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_runtime_allow(&pdev->dev);
+
r = i2c_dw_probe(dev);
- if (r)
+ if (r) {
+ pm_runtime_forbid(&pdev->dev);
return r;
+ }
- pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
- pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_put_autosuspend(&pdev->dev);
- pm_runtime_allow(&pdev->dev);
return 0;
}
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 4/4] i2c: designware: remove runtime suspend prevention during registration
2016-04-14 12:53 ` Jisheng Zhang
(?)
@ 2016-04-14 12:53 ` Jisheng Zhang
-1 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-14 12:53 UTC (permalink / raw)
To: jarkko.nikula, andriy.shevchenko, mika.westerberg, wsa
Cc: linux-i2c, linux-kernel, linux-arm-kernel, Jisheng Zhang
Now all users make sure there won't be runtime suspend when calling
i2c_dw_probe(), so we can remove the prevention code now.
Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
drivers/i2c/busses/i2c-designware-core.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 99b54be..4255eaa 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -881,17 +881,9 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
return r;
}
- /*
- * Increment PM usage count during adapter registration in order to
- * avoid possible spurious runtime suspend when adapter device is
- * registered to the device core and immediate resume in case bus has
- * registered I2C slaves that do I2C transfers in their probe.
- */
- pm_runtime_get_noresume(dev->dev);
r = i2c_add_numbered_adapter(adap);
if (r)
dev_err(dev->dev, "failure adding adapter: %d\n", r);
- pm_runtime_put_noidle(dev->dev);
return r;
}
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 4/4] i2c: designware: remove runtime suspend prevention during registration
@ 2016-04-14 12:53 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-14 12:53 UTC (permalink / raw)
To: jarkko.nikula, andriy.shevchenko, mika.westerberg, wsa
Cc: linux-i2c, linux-kernel, linux-arm-kernel, Jisheng Zhang
Now all users make sure there won't be runtime suspend when calling
i2c_dw_probe(), so we can remove the prevention code now.
Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
drivers/i2c/busses/i2c-designware-core.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 99b54be..4255eaa 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -881,17 +881,9 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
return r;
}
- /*
- * Increment PM usage count during adapter registration in order to
- * avoid possible spurious runtime suspend when adapter device is
- * registered to the device core and immediate resume in case bus has
- * registered I2C slaves that do I2C transfers in their probe.
- */
- pm_runtime_get_noresume(dev->dev);
r = i2c_add_numbered_adapter(adap);
if (r)
dev_err(dev->dev, "failure adding adapter: %d\n", r);
- pm_runtime_put_noidle(dev->dev);
return r;
}
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 4/4] i2c: designware: remove runtime suspend prevention during registration
@ 2016-04-14 12:53 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-14 12:53 UTC (permalink / raw)
To: linux-arm-kernel
Now all users make sure there won't be runtime suspend when calling
i2c_dw_probe(), so we can remove the prevention code now.
Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
drivers/i2c/busses/i2c-designware-core.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 99b54be..4255eaa 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -881,17 +881,9 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
return r;
}
- /*
- * Increment PM usage count during adapter registration in order to
- * avoid possible spurious runtime suspend when adapter device is
- * registered to the device core and immediate resume in case bus has
- * registered I2C slaves that do I2C transfers in their probe.
- */
- pm_runtime_get_noresume(dev->dev);
r = i2c_add_numbered_adapter(adap);
if (r)
dev_err(dev->dev, "failure adding adapter: %d\n", r);
- pm_runtime_put_noidle(dev->dev);
return r;
}
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare
2016-04-14 12:53 ` Jisheng Zhang
@ 2016-04-20 12:55 ` Jarkko Nikula
-1 siblings, 0 replies; 39+ messages in thread
From: Jarkko Nikula @ 2016-04-20 12:55 UTC (permalink / raw)
To: Jisheng Zhang, andriy.shevchenko, mika.westerberg, wsa
Cc: linux-i2c, linux-kernel, linux-arm-kernel
On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
> disable and unprepare the clk, otherwise the clk enable and prepare
> is left unbalanced.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
> drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 00f9e99..1488cea 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -268,6 +268,8 @@ rpm_disable:
> pm_runtime_put_noidle(&pdev->dev);
> }
>
> + i2c_dw_plat_prepare_clk(dev, false);
> +
> return r;
> }
>
This is a bit unclear to me does devm_clk_get take care of clk disabling
in case of probe error or driver removal?
I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions
(devm_*)") removed it but at quick look drivers/clk/clk-devres.c:
devm_clk_release() calls only clk_put and I don't see disable is done
down the path.
--
Jarkko
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare
@ 2016-04-20 12:55 ` Jarkko Nikula
0 siblings, 0 replies; 39+ messages in thread
From: Jarkko Nikula @ 2016-04-20 12:55 UTC (permalink / raw)
To: linux-arm-kernel
On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
> disable and unprepare the clk, otherwise the clk enable and prepare
> is left unbalanced.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
> drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 00f9e99..1488cea 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -268,6 +268,8 @@ rpm_disable:
> pm_runtime_put_noidle(&pdev->dev);
> }
>
> + i2c_dw_plat_prepare_clk(dev, false);
> +
> return r;
> }
>
This is a bit unclear to me does devm_clk_get take care of clk disabling
in case of probe error or driver removal?
I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions
(devm_*)") removed it but at quick look drivers/clk/clk-devres.c:
devm_clk_release() calls only clk_put and I don't see disable is done
down the path.
--
Jarkko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/4] i2c: designware-pcidrv: enable RuntimePM before registering to the core
2016-04-14 12:53 ` Jisheng Zhang
@ 2016-04-20 13:03 ` Jarkko Nikula
-1 siblings, 0 replies; 39+ messages in thread
From: Jarkko Nikula @ 2016-04-20 13:03 UTC (permalink / raw)
To: Jisheng Zhang, andriy.shevchenko, mika.westerberg, wsa
Cc: linux-i2c, linux-kernel, linux-arm-kernel
On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> As pointed out by commit 36d48fb5766a ("i2c: designware-platdrv: enable
> RuntimePM before registering to the core"), "The core may register
> clients attached to this master which may use funtionality from the
> master", so enable RuntimePM before registering to the core.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
> drivers/i2c/busses/i2c-designware-pcidrv.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index 7368be0..41a01a7 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -248,14 +248,17 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
> ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
> adap->nr = controller->bus_num;
>
> + pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> + pm_runtime_use_autosuspend(&pdev->dev);
> + pm_runtime_allow(&pdev->dev);
> +
> r = i2c_dw_probe(dev);
> - if (r)
> + if (r) {
> + pm_runtime_forbid(&pdev->dev);
> return r;
> + }
>
> - pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> - pm_runtime_use_autosuspend(&pdev->dev);
> pm_runtime_put_autosuspend(&pdev->dev);
> - pm_runtime_allow(&pdev->dev);
>
Last time I checked this I didn't see urgent reason for the move because
runtime PM is enabled for PCI devices by default via pci_device_add() ->
pci_init_capabilities() -> pci_pm_init() -> pm_runtime_enable().
--
Jarkko
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 3/4] i2c: designware-pcidrv: enable RuntimePM before registering to the core
@ 2016-04-20 13:03 ` Jarkko Nikula
0 siblings, 0 replies; 39+ messages in thread
From: Jarkko Nikula @ 2016-04-20 13:03 UTC (permalink / raw)
To: linux-arm-kernel
On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> As pointed out by commit 36d48fb5766a ("i2c: designware-platdrv: enable
> RuntimePM before registering to the core"), "The core may register
> clients attached to this master which may use funtionality from the
> master", so enable RuntimePM before registering to the core.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
> drivers/i2c/busses/i2c-designware-pcidrv.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index 7368be0..41a01a7 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -248,14 +248,17 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
> ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
> adap->nr = controller->bus_num;
>
> + pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> + pm_runtime_use_autosuspend(&pdev->dev);
> + pm_runtime_allow(&pdev->dev);
> +
> r = i2c_dw_probe(dev);
> - if (r)
> + if (r) {
> + pm_runtime_forbid(&pdev->dev);
> return r;
> + }
>
> - pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> - pm_runtime_use_autosuspend(&pdev->dev);
> pm_runtime_put_autosuspend(&pdev->dev);
> - pm_runtime_allow(&pdev->dev);
>
Last time I checked this I didn't see urgent reason for the move because
runtime PM is enabled for PCI devices by default via pci_device_add() ->
pci_init_capabilities() -> pci_pm_init() -> pm_runtime_enable().
--
Jarkko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization
2016-04-14 12:53 ` Jisheng Zhang
@ 2016-04-20 13:53 ` Jarkko Nikula
-1 siblings, 0 replies; 39+ messages in thread
From: Jarkko Nikula @ 2016-04-20 13:53 UTC (permalink / raw)
To: Jisheng Zhang, andriy.shevchenko, mika.westerberg, wsa
Cc: linux-i2c, linux-kernel, linux-arm-kernel
On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> When pm_runtime_enable() was being called, the device's usage counter
> was 0, causing the PM layer to runtime-suspend the device. We then
> went on to call i2c_dw_probe() on a suspended device, which could hung.
>
> Fix this by incrementing the usage counter before pm_runtime_enable().
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
> drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index d656657..00f9e99 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> if (dev->pm_runtime_disabled) {
> pm_runtime_forbid(&pdev->dev);
> } else {
> + pm_runtime_get_noresume(&pdev->dev);
> pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> pm_runtime_use_autosuspend(&pdev->dev);
> pm_runtime_set_active(&pdev->dev);
pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend
as far as I understand which made me thinking if there is some other
issue that cause the symptoms what you are seeing? Like race with
runtime PM or similar.
--
Jarkko
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization
@ 2016-04-20 13:53 ` Jarkko Nikula
0 siblings, 0 replies; 39+ messages in thread
From: Jarkko Nikula @ 2016-04-20 13:53 UTC (permalink / raw)
To: linux-arm-kernel
On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> When pm_runtime_enable() was being called, the device's usage counter
> was 0, causing the PM layer to runtime-suspend the device. We then
> went on to call i2c_dw_probe() on a suspended device, which could hung.
>
> Fix this by incrementing the usage counter before pm_runtime_enable().
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
> drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index d656657..00f9e99 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> if (dev->pm_runtime_disabled) {
> pm_runtime_forbid(&pdev->dev);
> } else {
> + pm_runtime_get_noresume(&pdev->dev);
> pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> pm_runtime_use_autosuspend(&pdev->dev);
> pm_runtime_set_active(&pdev->dev);
pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend
as far as I understand which made me thinking if there is some other
issue that cause the symptoms what you are seeing? Like race with
runtime PM or similar.
--
Jarkko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare
2016-04-20 12:55 ` Jarkko Nikula
@ 2016-04-20 14:16 ` Andy Shevchenko
-1 siblings, 0 replies; 39+ messages in thread
From: Andy Shevchenko @ 2016-04-20 14:16 UTC (permalink / raw)
To: Jarkko Nikula, Jisheng Zhang, mika.westerberg, wsa
Cc: linux-i2c, linux-kernel, linux-arm-kernel
On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> >
> > If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
> > disable and unprepare the clk, otherwise the clk enable and prepare
> > is left unbalanced.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> > drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> > b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index 00f9e99..1488cea 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -268,6 +268,8 @@ rpm_disable:
> > pm_runtime_put_noidle(&pdev->dev);
> > }
> >
> > + i2c_dw_plat_prepare_clk(dev, false);
> > +
> > return r;
> > }
> >
> This is a bit unclear to me does devm_clk_get take care of clk
> disabling
> in case of probe error or driver removal?
>
> I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions
> (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:
> devm_clk_release() calls only clk_put and I don't see disable is done
> down the path.
The following is a mistake of the mentioned patch.
- clk_disable_unprepare(dev->clk);
I did at the same mistake in dw_dmac driver which had been fixed later
in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
function").
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare
@ 2016-04-20 14:16 ` Andy Shevchenko
0 siblings, 0 replies; 39+ messages in thread
From: Andy Shevchenko @ 2016-04-20 14:16 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> >
> > If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
> > disable and unprepare the clk, otherwise the clk enable and prepare
> > is left unbalanced.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> > ? drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
> > ? 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> > b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index 00f9e99..1488cea 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -268,6 +268,8 @@ rpm_disable:
> > ?? pm_runtime_put_noidle(&pdev->dev);
> > ?? }
> >
> > + i2c_dw_plat_prepare_clk(dev, false);
> > +
> > ?? return r;
> > ? }
> >
> This is a bit unclear to me does devm_clk_get take care of clk
> disabling?
> in case of probe error or driver removal?
>
> I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions?
> (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:?
> devm_clk_release() calls only clk_put and I don't see disable is done?
> down the path.
The following is a mistake of the mentioned patch.
-???????clk_disable_unprepare(dev->clk);
I did at the same mistake in dw_dmac driver which had been fixed later
in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
function").
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare
2016-04-20 14:16 ` Andy Shevchenko
(?)
@ 2016-04-21 2:40 ` Jisheng Zhang
-1 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-21 2:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jarkko Nikula, mika.westerberg, wsa, linux-i2c, linux-kernel,
linux-arm-kernel
Dear Jarkko, Andy,
On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:
> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
> > On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> > >
> > > If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
> > > disable and unprepare the clk, otherwise the clk enable and prepare
> > > is left unbalanced.
> > >
> > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > > ---
> > > drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > index 00f9e99..1488cea 100644
> > > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > @@ -268,6 +268,8 @@ rpm_disable:
> > > pm_runtime_put_noidle(&pdev->dev);
> > > }
> > >
> > > + i2c_dw_plat_prepare_clk(dev, false);
> > > +
> > > return r;
> > > }
> > >
> > This is a bit unclear to me does devm_clk_get take care of clk
> > disabling
> > in case of probe error or driver removal?
> >
> > I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions
> > (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:
> > devm_clk_release() calls only clk_put and I don't see disable is done
> > down the path.
>
> The following is a mistake of the mentioned patch.
> - clk_disable_unprepare(dev->clk);
>
> I did at the same mistake in dw_dmac driver which had been fixed later
> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
> function").
>
As Andy pointed out, managed devm_clk_get can only automatically put clk
but doesn't unprepare and disable the clk
Thanks,
Jisheng
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare
@ 2016-04-21 2:40 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-21 2:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: wsa, linux-kernel, Jarkko Nikula, linux-i2c, mika.westerberg,
linux-arm-kernel
Dear Jarkko, Andy,
On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:
> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
> > On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> > >
> > > If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
> > > disable and unprepare the clk, otherwise the clk enable and prepare
> > > is left unbalanced.
> > >
> > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > > ---
> > > drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > index 00f9e99..1488cea 100644
> > > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > @@ -268,6 +268,8 @@ rpm_disable:
> > > pm_runtime_put_noidle(&pdev->dev);
> > > }
> > >
> > > + i2c_dw_plat_prepare_clk(dev, false);
> > > +
> > > return r;
> > > }
> > >
> > This is a bit unclear to me does devm_clk_get take care of clk
> > disabling
> > in case of probe error or driver removal?
> >
> > I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions
> > (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:
> > devm_clk_release() calls only clk_put and I don't see disable is done
> > down the path.
>
> The following is a mistake of the mentioned patch.
> - clk_disable_unprepare(dev->clk);
>
> I did at the same mistake in dw_dmac driver which had been fixed later
> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
> function").
>
As Andy pointed out, managed devm_clk_get can only automatically put clk
but doesn't unprepare and disable the clk
Thanks,
Jisheng
_______________________________________________
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] 39+ messages in thread
* [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare
@ 2016-04-21 2:40 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-21 2:40 UTC (permalink / raw)
To: linux-arm-kernel
Dear Jarkko, Andy,
On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:
> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
> > On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> > >
> > > If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
> > > disable and unprepare the clk, otherwise the clk enable and prepare
> > > is left unbalanced.
> > >
> > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > > ---
> > > ? drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
> > > ? 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > index 00f9e99..1488cea 100644
> > > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > @@ -268,6 +268,8 @@ rpm_disable:
> > > ?? pm_runtime_put_noidle(&pdev->dev);
> > > ?? }
> > >
> > > + i2c_dw_plat_prepare_clk(dev, false);
> > > +
> > > ?? return r;
> > > ? }
> > >
> > This is a bit unclear to me does devm_clk_get take care of clk
> > disabling?
> > in case of probe error or driver removal?
> >
> > I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions?
> > (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:?
> > devm_clk_release() calls only clk_put and I don't see disable is done?
> > down the path.
>
> The following is a mistake of the mentioned patch.
> -???????clk_disable_unprepare(dev->clk);
>
> I did at the same mistake in dw_dmac driver which had been fixed later
> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
> function").
>
As Andy pointed out, managed devm_clk_get can only automatically put clk
but doesn't unprepare and disable the clk
Thanks,
Jisheng
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization
2016-04-20 13:53 ` Jarkko Nikula
(?)
@ 2016-04-21 3:08 ` Jisheng Zhang
-1 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-21 3:08 UTC (permalink / raw)
To: Jarkko Nikula
Cc: andriy.shevchenko, mika.westerberg, wsa, linux-i2c, linux-kernel,
linux-arm-kernel
Dear Jarkko,
On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:
> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> > When pm_runtime_enable() was being called, the device's usage counter
> > was 0, causing the PM layer to runtime-suspend the device. We then
> > went on to call i2c_dw_probe() on a suspended device, which could hung.
> >
> > Fix this by incrementing the usage counter before pm_runtime_enable().
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> > drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index d656657..00f9e99 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> > if (dev->pm_runtime_disabled) {
> > pm_runtime_forbid(&pdev->dev);
> > } else {
> > + pm_runtime_get_noresume(&pdev->dev);
> > pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> > pm_runtime_use_autosuspend(&pdev->dev);
> > pm_runtime_set_active(&pdev->dev);
>
> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend
> as far as I understand which made me thinking if there is some other
FWICT, on arm DT platform, the device usage counter is zero at the beginning,
once we enable rpm, the i2c have chance to runtime suspend.
Thanks,
Jisheng
> issue that cause the symptoms what you are seeing? Like race with
> runtime PM or similar.
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization
@ 2016-04-21 3:08 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-21 3:08 UTC (permalink / raw)
To: Jarkko Nikula
Cc: andriy.shevchenko, mika.westerberg, wsa, linux-i2c, linux-kernel,
linux-arm-kernel
Dear Jarkko,
On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:
> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> > When pm_runtime_enable() was being called, the device's usage counter
> > was 0, causing the PM layer to runtime-suspend the device. We then
> > went on to call i2c_dw_probe() on a suspended device, which could hung.
> >
> > Fix this by incrementing the usage counter before pm_runtime_enable().
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> > drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index d656657..00f9e99 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> > if (dev->pm_runtime_disabled) {
> > pm_runtime_forbid(&pdev->dev);
> > } else {
> > + pm_runtime_get_noresume(&pdev->dev);
> > pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> > pm_runtime_use_autosuspend(&pdev->dev);
> > pm_runtime_set_active(&pdev->dev);
>
> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend
> as far as I understand which made me thinking if there is some other
FWICT, on arm DT platform, the device usage counter is zero at the beginning,
once we enable rpm, the i2c have chance to runtime suspend.
Thanks,
Jisheng
> issue that cause the symptoms what you are seeing? Like race with
> runtime PM or similar.
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization
@ 2016-04-21 3:08 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-21 3:08 UTC (permalink / raw)
To: linux-arm-kernel
Dear Jarkko,
On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:
> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> > When pm_runtime_enable() was being called, the device's usage counter
> > was 0, causing the PM layer to runtime-suspend the device. We then
> > went on to call i2c_dw_probe() on a suspended device, which could hung.
> >
> > Fix this by incrementing the usage counter before pm_runtime_enable().
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> > drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index d656657..00f9e99 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> > if (dev->pm_runtime_disabled) {
> > pm_runtime_forbid(&pdev->dev);
> > } else {
> > + pm_runtime_get_noresume(&pdev->dev);
> > pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> > pm_runtime_use_autosuspend(&pdev->dev);
> > pm_runtime_set_active(&pdev->dev);
>
> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend
> as far as I understand which made me thinking if there is some other
FWICT, on arm DT platform, the device usage counter is zero at the beginning,
once we enable rpm, the i2c have chance to runtime suspend.
Thanks,
Jisheng
> issue that cause the symptoms what you are seeing? Like race with
> runtime PM or similar.
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare
2016-04-21 2:40 ` Jisheng Zhang
@ 2016-04-21 7:39 ` Jarkko Nikula
-1 siblings, 0 replies; 39+ messages in thread
From: Jarkko Nikula @ 2016-04-21 7:39 UTC (permalink / raw)
To: Jisheng Zhang, Andy Shevchenko
Cc: mika.westerberg, wsa, linux-i2c, linux-kernel, linux-arm-kernel
On 04/21/2016 05:40 AM, Jisheng Zhang wrote:
> Dear Jarkko, Andy,
>
> On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:
>
>> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
>>> I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions
>>> (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:
>>> devm_clk_release() calls only clk_put and I don't see disable is done
>>> down the path.
>>
>> The following is a mistake of the mentioned patch.
>> - clk_disable_unprepare(dev->clk);
>>
>> I did at the same mistake in dw_dmac driver which had been fixed later
>> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
>> function").
>>
>
> As Andy pointed out, managed devm_clk_get can only automatically put clk
> but doesn't unprepare and disable the clk
>
Ok, then it makes sense to move this fix first in the series and queue
for stable v4.5+. Then another from you, Andy or me for kernels before
b33af11de236 ("i2c: designware: Do not require clock when SSCN and FFCN
are provided") that introduced the i2c_dw_plat_prepare_clk().
--
Jarkko
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare
@ 2016-04-21 7:39 ` Jarkko Nikula
0 siblings, 0 replies; 39+ messages in thread
From: Jarkko Nikula @ 2016-04-21 7:39 UTC (permalink / raw)
To: linux-arm-kernel
On 04/21/2016 05:40 AM, Jisheng Zhang wrote:
> Dear Jarkko, Andy,
>
> On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:
>
>> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
>>> I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions
>>> (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:
>>> devm_clk_release() calls only clk_put and I don't see disable is done
>>> down the path.
>>
>> The following is a mistake of the mentioned patch.
>> - clk_disable_unprepare(dev->clk);
>>
>> I did at the same mistake in dw_dmac driver which had been fixed later
>> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
>> function").
>>
>
> As Andy pointed out, managed devm_clk_get can only automatically put clk
> but doesn't unprepare and disable the clk
>
Ok, then it makes sense to move this fix first in the series and queue
for stable v4.5+. Then another from you, Andy or me for kernels before
b33af11de236 ("i2c: designware: Do not require clock when SSCN and FFCN
are provided") that introduced the i2c_dw_plat_prepare_clk().
--
Jarkko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization
2016-04-21 3:08 ` Jisheng Zhang
@ 2016-04-21 7:48 ` Jarkko Nikula
-1 siblings, 0 replies; 39+ messages in thread
From: Jarkko Nikula @ 2016-04-21 7:48 UTC (permalink / raw)
To: Jisheng Zhang
Cc: andriy.shevchenko, mika.westerberg, wsa, linux-i2c, linux-kernel,
linux-arm-kernel
Hi
On 04/21/2016 06:08 AM, Jisheng Zhang wrote:
> Dear Jarkko,
>
> On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:
>
>> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
>>> When pm_runtime_enable() was being called, the device's usage counter
>>> was 0, causing the PM layer to runtime-suspend the device. We then
>>> went on to call i2c_dw_probe() on a suspended device, which could hung.
>>>
>>> Fix this by incrementing the usage counter before pm_runtime_enable().
>>>
>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>>> ---
>>> drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
>>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> index d656657..00f9e99 100644
>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>>> if (dev->pm_runtime_disabled) {
>>> pm_runtime_forbid(&pdev->dev);
>>> } else {
>>> + pm_runtime_get_noresume(&pdev->dev);
>>> pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
>>> pm_runtime_use_autosuspend(&pdev->dev);
>>> pm_runtime_set_active(&pdev->dev);
>>
>> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend
>> as far as I understand which made me thinking if there is some other
>
> FWICT, on arm DT platform, the device usage counter is zero at the beginning,
> once we enable rpm, the i2c have chance to runtime suspend.
>
Yes it is same also for ACPI platforms. What I like to understand what
is actually causing the runtime suspend in your case.
If I add delay longer than 1 second between pm_runtime_enable() and
i2c_dw_probe() suspends still happens only after probe finishes. It will
be triggered by the drivers/base/dd.c: driver_probe_device() when code
calls the pm_request_idle().
I'm wondering is it possible there is a race with deferred probe etc.
that is causing the issue you are seeing.
--
Jarkko
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization
@ 2016-04-21 7:48 ` Jarkko Nikula
0 siblings, 0 replies; 39+ messages in thread
From: Jarkko Nikula @ 2016-04-21 7:48 UTC (permalink / raw)
To: linux-arm-kernel
Hi
On 04/21/2016 06:08 AM, Jisheng Zhang wrote:
> Dear Jarkko,
>
> On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:
>
>> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
>>> When pm_runtime_enable() was being called, the device's usage counter
>>> was 0, causing the PM layer to runtime-suspend the device. We then
>>> went on to call i2c_dw_probe() on a suspended device, which could hung.
>>>
>>> Fix this by incrementing the usage counter before pm_runtime_enable().
>>>
>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>>> ---
>>> drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
>>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> index d656657..00f9e99 100644
>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>>> if (dev->pm_runtime_disabled) {
>>> pm_runtime_forbid(&pdev->dev);
>>> } else {
>>> + pm_runtime_get_noresume(&pdev->dev);
>>> pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
>>> pm_runtime_use_autosuspend(&pdev->dev);
>>> pm_runtime_set_active(&pdev->dev);
>>
>> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend
>> as far as I understand which made me thinking if there is some other
>
> FWICT, on arm DT platform, the device usage counter is zero at the beginning,
> once we enable rpm, the i2c have chance to runtime suspend.
>
Yes it is same also for ACPI platforms. What I like to understand what
is actually causing the runtime suspend in your case.
If I add delay longer than 1 second between pm_runtime_enable() and
i2c_dw_probe() suspends still happens only after probe finishes. It will
be triggered by the drivers/base/dd.c: driver_probe_device() when code
calls the pm_request_idle().
I'm wondering is it possible there is a race with deferred probe etc.
that is causing the issue you are seeing.
--
Jarkko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization
2016-04-21 7:48 ` Jarkko Nikula
(?)
@ 2016-04-21 8:15 ` Jisheng Zhang
-1 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-21 8:15 UTC (permalink / raw)
To: Jarkko Nikula
Cc: andriy.shevchenko, mika.westerberg, wsa, linux-i2c, linux-kernel,
linux-arm-kernel
On Thu, 21 Apr 2016 10:48:56 +0300
Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> Hi
>
> On 04/21/2016 06:08 AM, Jisheng Zhang wrote:
> > Dear Jarkko,
> >
> > On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:
> >
> >> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> >>> When pm_runtime_enable() was being called, the device's usage counter
> >>> was 0, causing the PM layer to runtime-suspend the device. We then
> >>> went on to call i2c_dw_probe() on a suspended device, which could hung.
> >>>
> >>> Fix this by incrementing the usage counter before pm_runtime_enable().
> >>>
> >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >>> ---
> >>> drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
> >>> 1 file changed, 13 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> index d656657..00f9e99 100644
> >>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> >>> if (dev->pm_runtime_disabled) {
> >>> pm_runtime_forbid(&pdev->dev);
> >>> } else {
> >>> + pm_runtime_get_noresume(&pdev->dev);
> >>> pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> >>> pm_runtime_use_autosuspend(&pdev->dev);
> >>> pm_runtime_set_active(&pdev->dev);
> >>
> >> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend
> >> as far as I understand which made me thinking if there is some other
> >
> > FWICT, on arm DT platform, the device usage counter is zero at the beginning,
> > once we enable rpm, the i2c have chance to runtime suspend.
> >
> Yes it is same also for ACPI platforms. What I like to understand what
> is actually causing the runtime suspend in your case.
OK, got your point.
>
> If I add delay longer than 1 second between pm_runtime_enable() and
> i2c_dw_probe() suspends still happens only after probe finishes. It will
> be triggered by the drivers/base/dd.c: driver_probe_device() when code
> calls the pm_request_idle().
>
> I'm wondering is it possible there is a race with deferred probe etc.
> that is causing the issue you are seeing.
Hmm, I do see there's deferred probe for the i2c hosts. something like:
[ 0.177338] platform f7fcc000.i2c: Driver i2c_designware requests probe deferral
Is it possible as this:
probe => driver return EPROBE_DEFER => pm_request_idle() => probe again
=> since the device usage count is zero, once rpm is enabled, we have
chance to suspend?
And from another side, is it reasonable to always increase the usage count
until probe is done?
Thanks,
Jisheng
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization
@ 2016-04-21 8:15 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-21 8:15 UTC (permalink / raw)
To: Jarkko Nikula
Cc: andriy.shevchenko, mika.westerberg, wsa, linux-i2c, linux-kernel,
linux-arm-kernel
On Thu, 21 Apr 2016 10:48:56 +0300
Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> Hi
>
> On 04/21/2016 06:08 AM, Jisheng Zhang wrote:
> > Dear Jarkko,
> >
> > On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:
> >
> >> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> >>> When pm_runtime_enable() was being called, the device's usage counter
> >>> was 0, causing the PM layer to runtime-suspend the device. We then
> >>> went on to call i2c_dw_probe() on a suspended device, which could hung.
> >>>
> >>> Fix this by incrementing the usage counter before pm_runtime_enable().
> >>>
> >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >>> ---
> >>> drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
> >>> 1 file changed, 13 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> index d656657..00f9e99 100644
> >>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> >>> if (dev->pm_runtime_disabled) {
> >>> pm_runtime_forbid(&pdev->dev);
> >>> } else {
> >>> + pm_runtime_get_noresume(&pdev->dev);
> >>> pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> >>> pm_runtime_use_autosuspend(&pdev->dev);
> >>> pm_runtime_set_active(&pdev->dev);
> >>
> >> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend
> >> as far as I understand which made me thinking if there is some other
> >
> > FWICT, on arm DT platform, the device usage counter is zero at the beginning,
> > once we enable rpm, the i2c have chance to runtime suspend.
> >
> Yes it is same also for ACPI platforms. What I like to understand what
> is actually causing the runtime suspend in your case.
OK, got your point.
>
> If I add delay longer than 1 second between pm_runtime_enable() and
> i2c_dw_probe() suspends still happens only after probe finishes. It will
> be triggered by the drivers/base/dd.c: driver_probe_device() when code
> calls the pm_request_idle().
>
> I'm wondering is it possible there is a race with deferred probe etc.
> that is causing the issue you are seeing.
Hmm, I do see there's deferred probe for the i2c hosts. something like:
[ 0.177338] platform f7fcc000.i2c: Driver i2c_designware requests probe deferral
Is it possible as this:
probe => driver return EPROBE_DEFER => pm_request_idle() => probe again
=> since the device usage count is zero, once rpm is enabled, we have
chance to suspend?
And from another side, is it reasonable to always increase the usage count
until probe is done?
Thanks,
Jisheng
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization
@ 2016-04-21 8:15 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-21 8:15 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 21 Apr 2016 10:48:56 +0300
Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> Hi
>
> On 04/21/2016 06:08 AM, Jisheng Zhang wrote:
> > Dear Jarkko,
> >
> > On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:
> >
> >> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> >>> When pm_runtime_enable() was being called, the device's usage counter
> >>> was 0, causing the PM layer to runtime-suspend the device. We then
> >>> went on to call i2c_dw_probe() on a suspended device, which could hung.
> >>>
> >>> Fix this by incrementing the usage counter before pm_runtime_enable().
> >>>
> >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >>> ---
> >>> drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
> >>> 1 file changed, 13 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> index d656657..00f9e99 100644
> >>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> >>> if (dev->pm_runtime_disabled) {
> >>> pm_runtime_forbid(&pdev->dev);
> >>> } else {
> >>> + pm_runtime_get_noresume(&pdev->dev);
> >>> pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> >>> pm_runtime_use_autosuspend(&pdev->dev);
> >>> pm_runtime_set_active(&pdev->dev);
> >>
> >> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend
> >> as far as I understand which made me thinking if there is some other
> >
> > FWICT, on arm DT platform, the device usage counter is zero at the beginning,
> > once we enable rpm, the i2c have chance to runtime suspend.
> >
> Yes it is same also for ACPI platforms. What I like to understand what
> is actually causing the runtime suspend in your case.
OK, got your point.
>
> If I add delay longer than 1 second between pm_runtime_enable() and
> i2c_dw_probe() suspends still happens only after probe finishes. It will
> be triggered by the drivers/base/dd.c: driver_probe_device() when code
> calls the pm_request_idle().
>
> I'm wondering is it possible there is a race with deferred probe etc.
> that is causing the issue you are seeing.
Hmm, I do see there's deferred probe for the i2c hosts. something like:
[ 0.177338] platform f7fcc000.i2c: Driver i2c_designware requests probe deferral
Is it possible as this:
probe => driver return EPROBE_DEFER => pm_request_idle() => probe again
=> since the device usage count is zero, once rpm is enabled, we have
chance to suspend?
And from another side, is it reasonable to always increase the usage count
until probe is done?
Thanks,
Jisheng
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare
2016-04-21 7:39 ` Jarkko Nikula
(?)
@ 2016-04-21 8:19 ` Jisheng Zhang
-1 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-21 8:19 UTC (permalink / raw)
To: Jarkko Nikula
Cc: Andy Shevchenko, mika.westerberg, wsa, linux-i2c, linux-kernel,
linux-arm-kernel
Dear Jarkko,
On Thu, 21 Apr 2016 10:39:50 +0300 Jarkko Nikula wrote:
> On 04/21/2016 05:40 AM, Jisheng Zhang wrote:
> > Dear Jarkko, Andy,
> >
> > On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:
> >
> >> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
> >>> I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions
> >>> (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:
> >>> devm_clk_release() calls only clk_put and I don't see disable is done
> >>> down the path.
> >>
> >> The following is a mistake of the mentioned patch.
> >> - clk_disable_unprepare(dev->clk);
> >>
> >> I did at the same mistake in dw_dmac driver which had been fixed later
> >> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
> >> function").
> >>
> >
> > As Andy pointed out, managed devm_clk_get can only automatically put clk
> > but doesn't unprepare and disable the clk
> >
> Ok, then it makes sense to move this fix first in the series and queue
> for stable v4.5+. Then another from you, Andy or me for kernels before
> b33af11de236 ("i2c: designware: Do not require clock when SSCN and FFCN
> are provided") that introduced the i2c_dw_plat_prepare_clk().
Sounds a good idea. Will send a separate fix for this purpose soon.
Thanks,
Jisheng
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare
@ 2016-04-21 8:19 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-21 8:19 UTC (permalink / raw)
To: Jarkko Nikula
Cc: Andy Shevchenko, mika.westerberg, wsa, linux-i2c, linux-kernel,
linux-arm-kernel
Dear Jarkko,
On Thu, 21 Apr 2016 10:39:50 +0300 Jarkko Nikula wrote:
> On 04/21/2016 05:40 AM, Jisheng Zhang wrote:
> > Dear Jarkko, Andy,
> >
> > On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:
> >
> >> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
> >>> I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions
> >>> (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:
> >>> devm_clk_release() calls only clk_put and I don't see disable is done
> >>> down the path.
> >>
> >> The following is a mistake of the mentioned patch.
> >> - clk_disable_unprepare(dev->clk);
> >>
> >> I did at the same mistake in dw_dmac driver which had been fixed later
> >> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
> >> function").
> >>
> >
> > As Andy pointed out, managed devm_clk_get can only automatically put clk
> > but doesn't unprepare and disable the clk
> >
> Ok, then it makes sense to move this fix first in the series and queue
> for stable v4.5+. Then another from you, Andy or me for kernels before
> b33af11de236 ("i2c: designware: Do not require clock when SSCN and FFCN
> are provided") that introduced the i2c_dw_plat_prepare_clk().
Sounds a good idea. Will send a separate fix for this purpose soon.
Thanks,
Jisheng
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare
@ 2016-04-21 8:19 ` Jisheng Zhang
0 siblings, 0 replies; 39+ messages in thread
From: Jisheng Zhang @ 2016-04-21 8:19 UTC (permalink / raw)
To: linux-arm-kernel
Dear Jarkko,
On Thu, 21 Apr 2016 10:39:50 +0300 Jarkko Nikula wrote:
> On 04/21/2016 05:40 AM, Jisheng Zhang wrote:
> > Dear Jarkko, Andy,
> >
> > On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:
> >
> >> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
> >>> I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions
> >>> (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:
> >>> devm_clk_release() calls only clk_put and I don't see disable is done
> >>> down the path.
> >>
> >> The following is a mistake of the mentioned patch.
> >> - clk_disable_unprepare(dev->clk);
> >>
> >> I did at the same mistake in dw_dmac driver which had been fixed later
> >> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
> >> function").
> >>
> >
> > As Andy pointed out, managed devm_clk_get can only automatically put clk
> > but doesn't unprepare and disable the clk
> >
> Ok, then it makes sense to move this fix first in the series and queue
> for stable v4.5+. Then another from you, Andy or me for kernels before
> b33af11de236 ("i2c: designware: Do not require clock when SSCN and FFCN
> are provided") that introduced the i2c_dw_plat_prepare_clk().
Sounds a good idea. Will send a separate fix for this purpose soon.
Thanks,
Jisheng
^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2016-04-21 8:23 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-14 12:53 [PATCH 0/4] i2c: designware: runtime pm fix and improve Jisheng Zhang
2016-04-14 12:53 ` Jisheng Zhang
2016-04-14 12:53 ` Jisheng Zhang
2016-04-14 12:53 ` [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization Jisheng Zhang
2016-04-14 12:53 ` Jisheng Zhang
2016-04-14 12:53 ` Jisheng Zhang
2016-04-20 13:53 ` Jarkko Nikula
2016-04-20 13:53 ` Jarkko Nikula
2016-04-21 3:08 ` Jisheng Zhang
2016-04-21 3:08 ` Jisheng Zhang
2016-04-21 3:08 ` Jisheng Zhang
2016-04-21 7:48 ` Jarkko Nikula
2016-04-21 7:48 ` Jarkko Nikula
2016-04-21 8:15 ` Jisheng Zhang
2016-04-21 8:15 ` Jisheng Zhang
2016-04-21 8:15 ` Jisheng Zhang
2016-04-14 12:53 ` [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare Jisheng Zhang
2016-04-14 12:53 ` Jisheng Zhang
2016-04-14 12:53 ` Jisheng Zhang
2016-04-20 12:55 ` Jarkko Nikula
2016-04-20 12:55 ` Jarkko Nikula
2016-04-20 14:16 ` Andy Shevchenko
2016-04-20 14:16 ` Andy Shevchenko
2016-04-21 2:40 ` Jisheng Zhang
2016-04-21 2:40 ` Jisheng Zhang
2016-04-21 2:40 ` Jisheng Zhang
2016-04-21 7:39 ` Jarkko Nikula
2016-04-21 7:39 ` Jarkko Nikula
2016-04-21 8:19 ` Jisheng Zhang
2016-04-21 8:19 ` Jisheng Zhang
2016-04-21 8:19 ` Jisheng Zhang
2016-04-14 12:53 ` [PATCH 3/4] i2c: designware-pcidrv: enable RuntimePM before registering to the core Jisheng Zhang
2016-04-14 12:53 ` Jisheng Zhang
2016-04-14 12:53 ` Jisheng Zhang
2016-04-20 13:03 ` Jarkko Nikula
2016-04-20 13:03 ` Jarkko Nikula
2016-04-14 12:53 ` [PATCH 4/4] i2c: designware: remove runtime suspend prevention during registration Jisheng Zhang
2016-04-14 12:53 ` Jisheng Zhang
2016-04-14 12:53 ` Jisheng Zhang
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.