[PATCH v3 0/2] ARM: dts: keystone-k2g: Add I2C support for 66AK2G

Add I2C support to 66AK2G. Primary requirement is to add PM
Runtime support to the driver.

This has been tested on following platforms by performing simple i2c test
such as i2c detect and reading on board i2c devices:
K2G GP evm
OMAPL138
K2L GP EVM

and boot tested on:
K2E GP EVM
K2HK GP EVM

Version 2 changes:
Moved ordering of pm runtime calls

Version 3 changes:
Dropped first patch since it was pulled in.
Remove clk = NULL statements
Fix error path

Franklin S Cooper Jr (2):
  i2c: davinci: Add PM Runtime Support
  dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm
    property

 .../devicetree/bindings/i2c/i2c-davinci.txt        | 12 ++++
 drivers/i2c/busses/i2c-davinci.c                   | 64 ++++++++++++++++++----
 2 files changed, 65 insertions(+), 11 deletions(-)

-- 
2.9.4.dirty

Add pm-domains property which is required for 66AK2Gx. Also document 66AK2G
unique clocks property usage.

Signed-off-by: Franklin S Cooper Jr <redacted>
Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Sekhar Nori <redacted>
---
 Documentation/devicetree/bindings/i2c/i2c-davinci.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
index 5b123e0..64e6e65 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt

@@ -6,6 +6,18 @@ davinci/keystone i2c interface contains.
 Required properties:
 - compatible: "ti,davinci-i2c" or "ti,keystone-i2c";
 - reg : Offset and length of the register set for the device
+- clocks: I2C functional clock phandle.
+	  For 66AK2G this property should be set per binding,
+	  Documentation/devicetree/bindings/clock/ti,sci-clk.txt
+
+SoC-specific Required Properties:
+
+The following are mandatory properties for Keystone 2 66AK2G SoCs only:
+
+- power-domains:	Should contain a phandle to a PM domain provider node
+			and an args specifier containing the I2C device id
+			value. This property is as per the binding,
+			Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
 
 Recommended properties :
 - interrupts : standard interrupt property.

-- 
2.9.4.dirty

66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
like other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime
is required to insure the power domain used by the specific I2C instance is
properly turned on along with its functional clock.

Signed-off-by: Franklin S Cooper Jr <redacted>
---
Version 3 changes:
Remove several statements that set clk to NULL
Fix error path

 drivers/i2c/busses/i2c-davinci.c | 64 +++++++++++++++++++++++++++++++++-------
 1 file changed, 53 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index b8c4353..57612cb 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c

@@ -36,6 +36,7 @@
 #include <linux/gpio.h>
 #include <linux/of_device.h>
 #include <linux/platform_data/i2c-davinci.h>
+#include <linux/pm_runtime.h>
 
 /* ----- global defines ----------------------------------------------- */
 

@@ -122,6 +123,9 @@
 /* set the SDA GPIO low */
 #define DAVINCI_I2C_DCLR_PDCLR1	BIT(1)
 
+/* timeout for pm runtime autosuspend */
+#define DAVINCI_I2C_PM_TIMEOUT	1000	/* ms */
+
 struct davinci_i2c_dev {
 	struct device           *dev;
 	void __iomem		*base;

@@ -541,10 +545,17 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
 
+	ret = pm_runtime_get_sync(dev->dev);
+	if (ret < 0) {
+		dev_err(dev->dev, "Failed to runtime_get device: %d\n", ret);
+		pm_runtime_put_noidle(dev->dev);
+		return ret;
+	}
+
 	ret = i2c_davinci_wait_bus_not_busy(dev);
 	if (ret < 0) {
 		dev_warn(dev->dev, "timeout waiting for bus ready\n");
-		return ret;
+		goto out;
 	}
 
 	for (i = 0; i < num; i++) {

@@ -552,14 +563,19 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
 			ret);
 		if (ret < 0)
-			return ret;
+			goto out;
 	}
 
+	ret = num;
 #ifdef CONFIG_CPU_FREQ
 	complete(&dev->xfr_complete);
 #endif
 
-	return num;
+out:
+	pm_runtime_mark_last_busy(dev->dev);
+	pm_runtime_put_autosuspend(dev->dev);
+
+	return ret;
 }
 
 static u32 i2c_davinci_func(struct i2c_adapter *adap)

@@ -599,6 +615,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
 	int count = 0;
 	u16 w;
 
+	if (pm_runtime_suspended(dev->dev))
+		return IRQ_NONE;
+
 	while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
 		dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
 		if (count++ == 100) {

@@ -802,12 +821,22 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 	dev->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(dev->clk))
 		return PTR_ERR(dev->clk);
-	clk_prepare_enable(dev->clk);
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	dev->base = devm_ioremap_resource(&pdev->dev, mem);
 	if (IS_ERR(dev->base)) {
-		r = PTR_ERR(dev->base);
+		return PTR_ERR(dev->base);
+	}
+
+	pm_runtime_set_autosuspend_delay(dev->dev,
+					 DAVINCI_I2C_PM_TIMEOUT);
+	pm_runtime_use_autosuspend(dev->dev);
+
+	pm_runtime_enable(dev->dev);
+
+	r = pm_runtime_get_sync(dev->dev);
+	if (r < 0) {
+		dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
 		goto err_unuse_clocks;
 	}
 

@@ -849,27 +878,40 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 	if (r)
 		goto err_unuse_clocks;
 
+	pm_runtime_mark_last_busy(dev->dev);
+	pm_runtime_put_autosuspend(dev->dev);
+
 	return 0;
 
 err_unuse_clocks:
-	clk_disable_unprepare(dev->clk);
-	dev->clk = NULL;
+	pm_runtime_dont_use_autosuspend(dev->dev);
+	pm_runtime_put_sync(dev->dev);
+	pm_runtime_disable(dev->dev);
+
 	return r;
 }
 
 static int davinci_i2c_remove(struct platform_device *pdev)
 {
 	struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
+	int ret;
 
 	i2c_davinci_cpufreq_deregister(dev);
 
 	i2c_del_adapter(&dev->adapter);
 
-	clk_disable_unprepare(dev->clk);
-	dev->clk = NULL;
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(&pdev->dev);
+		return ret;
+	}
 
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, 0);
 
+	pm_runtime_dont_use_autosuspend(dev->dev);
+	pm_runtime_put_sync(dev->dev);
+	pm_runtime_disable(dev->dev);
+
 	return 0;
 }
 

@@ -880,7 +922,6 @@ static int davinci_i2c_suspend(struct device *dev)
 
 	/* put I2C into reset */
 	davinci_i2c_reset_ctrl(i2c_dev, 0);
-	clk_disable_unprepare(i2c_dev->clk);
 
 	return 0;
 }

@@ -889,7 +930,6 @@ static int davinci_i2c_resume(struct device *dev)
 {
 	struct davinci_i2c_dev *i2c_dev = dev_get_drvdata(dev);
 
-	clk_prepare_enable(i2c_dev->clk);
 	/* take I2C out of reset */
 	davinci_i2c_reset_ctrl(i2c_dev, 1);
 

@@ -899,6 +939,8 @@ static int davinci_i2c_resume(struct device *dev)
 static const struct dev_pm_ops davinci_i2c_pm = {
 	.suspend        = davinci_i2c_suspend,
 	.resume         = davinci_i2c_resume,
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				      pm_runtime_force_resume)
 };
 
 #define davinci_i2c_pm_ops (&davinci_i2c_pm)

-- 
2.9.4.dirty

On Friday 08 September 2017 11:24 PM, Franklin S Cooper Jr wrote:

66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
like other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime

unlike ?

is required to insure the power domain used by the specific I2C instance is
properly turned on along with its functional clock.

Signed-off-by: Franklin S Cooper Jr <redacted>
---
Version 3 changes:
Remove several statements that set clk to NULL
Fix error path

 drivers/i2c/busses/i2c-davinci.c | 64 +++++++++++++++++++++++++++++++++-------
 1 file changed, 53 insertions(+), 11 deletions(-)

You end up doing a pm_runtime_put_sync() on failure here, instead of
pm_runtime_put_noidle() like rest of the patch.

May be handle this failure here instead of relying on the goto path.

Thanks,
Sekhar

On 09/11/2017 06:29 AM, Sekhar Nori wrote:

On Friday 08 September 2017 11:24 PM, Franklin S Cooper Jr wrote:

quoted

66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
like other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime

unlike ?

quoted

is required to insure the power domain used by the specific I2C instance is
properly turned on along with its functional clock.

Signed-off-by: Franklin S Cooper Jr <redacted>
---
Version 3 changes:
Remove several statements that set clk to NULL
Fix error path

 drivers/i2c/busses/i2c-davinci.c | 64 +++++++++++++++++++++++++++++++++-------
 1 file changed, 53 insertions(+), 11 deletions(-)

quoted

@@ -802,12 +821,22 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 	dev->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(dev->clk))
 		return PTR_ERR(dev->clk);
-	clk_prepare_enable(dev->clk);
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	dev->base = devm_ioremap_resource(&pdev->dev, mem);
 	if (IS_ERR(dev->base)) {
-		r = PTR_ERR(dev->base);
+		return PTR_ERR(dev->base);
+	}
+
+	pm_runtime_set_autosuspend_delay(dev->dev,
+					 DAVINCI_I2C_PM_TIMEOUT);
+	pm_runtime_use_autosuspend(dev->dev);
+
+	pm_runtime_enable(dev->dev);
+
+	r = pm_runtime_get_sync(dev->dev);
+	if (r < 0) {
+		dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
 		goto err_unuse_clocks;

You end up doing a pm_runtime_put_sync() on failure here, instead of
pm_runtime_put_noidle() like rest of the patch.

May be handle this failure here instead of relying on the goto path.

Ok

quoted

 	}
 

@@ -849,27 +878,40 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 	if (r)
 		goto err_unuse_clocks;
 
+	pm_runtime_mark_last_busy(dev->dev);
+	pm_runtime_put_autosuspend(dev->dev);
+
 	return 0;
 
 err_unuse_clocks:
-	clk_disable_unprepare(dev->clk);
-	dev->clk = NULL;
+	pm_runtime_dont_use_autosuspend(dev->dev);
+	pm_runtime_put_sync(dev->dev);
+	pm_runtime_disable(dev->dev);
+
 	return r;
 }

Thanks,
Sekhar

On 09/11/2017 03:07 PM, Franklin S Cooper Jr wrote:

On 09/11/2017 06:29 AM, Sekhar Nori wrote:

quoted

On Friday 08 September 2017 11:24 PM, Franklin S Cooper Jr wrote:

quoted

66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
like other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime

unlike ?

quoted

is required to insure the power domain used by the specific I2C instance is
properly turned on along with its functional clock.

Signed-off-by: Franklin S Cooper Jr <redacted>
---
Version 3 changes:
Remove several statements that set clk to NULL
Fix error path

  drivers/i2c/busses/i2c-davinci.c | 64 +++++++++++++++++++++++++++++++++-------
  1 file changed, 53 insertions(+), 11 deletions(-)

quoted

@@ -802,12 +821,22 @@ static int davinci_i2c_probe(struct platform_device *pdev)
  	dev->clk = devm_clk_get(&pdev->dev, NULL);
  	if (IS_ERR(dev->clk))
  		return PTR_ERR(dev->clk);
-	clk_prepare_enable(dev->clk);
  
  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	dev->base = devm_ioremap_resource(&pdev->dev, mem);
  	if (IS_ERR(dev->base)) {
-		r = PTR_ERR(dev->base);
+		return PTR_ERR(dev->base);
+	}
+
+	pm_runtime_set_autosuspend_delay(dev->dev,
+					 DAVINCI_I2C_PM_TIMEOUT);
+	pm_runtime_use_autosuspend(dev->dev);
+
+	pm_runtime_enable(dev->dev);
+
+	r = pm_runtime_get_sync(dev->dev);
+	if (r < 0) {
+		dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
  		goto err_unuse_clocks;

You end up doing a pm_runtime_put_sync() on failure here, instead of
pm_runtime_put_noidle() like rest of the patch.

May be handle this failure here instead of relying on the goto path.

Ok

I think, it's not necessary - pm_runtime_put_sync() can be used in this case
 (and used the same way in many other drivers).
In case, of failure in this place - pm_runtime_put_sync() will just decrement usage counter.

quoted

quoted

  	}
  

@@ -849,27 +878,40 @@ static int davinci_i2c_probe(struct platform_device *pdev)
  	if (r)
  		goto err_unuse_clocks;
  
+	pm_runtime_mark_last_busy(dev->dev);
+	pm_runtime_put_autosuspend(dev->dev);
+
  	return 0;
  
  err_unuse_clocks:
-	clk_disable_unprepare(dev->clk);
-	dev->clk = NULL;
+	pm_runtime_dont_use_autosuspend(dev->dev);
+	pm_runtime_put_sync(dev->dev);
+	pm_runtime_disable(dev->dev);
+
  	return r;
  }

Thanks,
Sekhar

-- 
regards,
-grygorii

On Tuesday 12 September 2017 01:41 AM, Grygorii Strashko wrote:

On 09/11/2017 03:07 PM, Franklin S Cooper Jr wrote:

quoted

On 09/11/2017 06:29 AM, Sekhar Nori wrote:

quoted

On Friday 08 September 2017 11:24 PM, Franklin S Cooper Jr wrote:

quoted

66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
like other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime

unlike ?

quoted

is required to insure the power domain used by the specific I2C instance is
properly turned on along with its functional clock.

Signed-off-by: Franklin S Cooper Jr <redacted>
---
Version 3 changes:
Remove several statements that set clk to NULL
Fix error path

  drivers/i2c/busses/i2c-davinci.c | 64 +++++++++++++++++++++++++++++++++-------
  1 file changed, 53 insertions(+), 11 deletions(-)

quoted

@@ -802,12 +821,22 @@ static int davinci_i2c_probe(struct platform_device *pdev)
  	dev->clk = devm_clk_get(&pdev->dev, NULL);
  	if (IS_ERR(dev->clk))
  		return PTR_ERR(dev->clk);
-	clk_prepare_enable(dev->clk);
  
  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	dev->base = devm_ioremap_resource(&pdev->dev, mem);
  	if (IS_ERR(dev->base)) {
-		r = PTR_ERR(dev->base);
+		return PTR_ERR(dev->base);
+	}
+
+	pm_runtime_set_autosuspend_delay(dev->dev,
+					 DAVINCI_I2C_PM_TIMEOUT);
+	pm_runtime_use_autosuspend(dev->dev);
+
+	pm_runtime_enable(dev->dev);
+
+	r = pm_runtime_get_sync(dev->dev);
+	if (r < 0) {
+		dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
  		goto err_unuse_clocks;

You end up doing a pm_runtime_put_sync() on failure here, instead of
pm_runtime_put_noidle() like rest of the patch.

May be handle this failure here instead of relying on the goto path.

Ok

I think, it's not necessary - pm_runtime_put_sync() can be used in this case
 (and used the same way in many other drivers).
In case, of failure in this place - pm_runtime_put_sync() will just decrement usage counter.

Can you please explain why this is the case? At least on DA850, I see
the runtime_idle callback invoked from rpm_idle() if
pm_runtime_put_sync() is used in the failure path.

Thanks
Sekhar